diff --git a/config.example.js b/config.example.js index 9513c55a..48e5ba09 100644 --- a/config.example.js +++ b/config.example.js @@ -93,8 +93,11 @@ module.exports = { // socket, and read by the /api/v1 endpoint. Both are optional. // // Bot logins beyond the `[bot]` suffix GitHub Apps carry (so pulldasher can - // fold/skip their PRs). Omit or [] for suffix-only detection. - bots: ['renovate-bot'], + // fold/skip their PRs). Omit or [] for suffix-only detection. ifixit-systems + // is our dependency-bumper and AI-agent account; Copilot is GitHub's coding + // agent (type Bot, no [bot] suffix). claude[bot] and the other suffixed apps + // are already detected without listing. + bots: ['renovate-bot', 'ifixit-systems', 'Copilot'], // Label title -> review-weight bucket (XS|S|M|L|XL). A PR carrying one of // these labels takes that weight instead of the diff-size guess, so an auto // weight label (and any manual override) drives the board and the API. Omit diff --git a/frontend-v2/src/app.tsx b/frontend-v2/src/app.tsx index 6b22dab5..3f3f463c 100644 --- a/frontend-v2/src/app.tsx +++ b/frontend-v2/src/app.tsx @@ -1,4 +1,12 @@ -import { useCallback, useEffect, useMemo, useRef, useState, type ReactNode } from 'react'; +import { + useCallback, + useDeferredValue, + useEffect, + useMemo, + useRef, + useState, + type ReactNode, +} from 'react'; import { ago, closedEpoch, n, pullKey, shortRepo } from '../../shared/format'; import type { ActionStateKey } from './model/actions'; import { actionState } from './model/actions'; @@ -8,6 +16,7 @@ import { matchesWeightFilter } from '../../shared/model/status'; import { buildParentLookup } from './model/stack'; import { buildReviewerPools, turnFor } from './model/rotation'; import { shipRelevance, shippedToast } from './model/shipped'; +import { notificationStale } from './model/notificationRelevance'; import type { Toast } from './model/toast'; import { claimReview, isSnoozed, usePulldasher } from './store'; import { primeScope, useScope } from './prefs'; @@ -15,7 +24,6 @@ import { useBoardHotkeys, useHeaderHeightVar } from './hooks'; import { ageRotDays, getSettings, type Settings as SettingsShape, useSettings } from './settings'; import { useNotifications } from './notifications'; import { ToastStack, useToasts } from './toasts'; -import { matchesQuery } from './model/query'; import { requestNames, useNames } from './model/names'; import { CRYO_KEY, isBotLogin, personHidden, repoHidden } from '../../shared/model/visibility'; import { reviewRequestedFrom } from './model/reviewers'; @@ -46,6 +54,7 @@ import { Team } from './views/Team'; import { Classic } from './views/Classic'; import { Ci } from './views/Ci'; import { Stats } from './views/Stats'; +import { Search } from './views/Search'; import { Settings } from './components/Settings'; export type { Lens }; @@ -263,6 +272,22 @@ export function App() { const [scope, setScope] = useScope(); const [lens, setLens] = useState(() => readHash().lens); const [query, setQuery] = useState(() => urlState.q); + // free text is find, not filter: any query switches the board to the Search + // lens (global, over open + closed), so the header box stops narrowing the + // current lens and starts finding across the whole board. + // + // The heavy part of a search is rendering the matched rows, and the first + // keystroke MOUNTS the Search view. useDeferredValue gives no deferral on a + // mount, so deferring inside Search left that first render synchronous, and a + // broad query (say "a") blocked the main thread ~350ms on the dummy board, + // more at prod scale, freezing the keystroke itself. Deferring the query HERE + // makes the view swap and its render a low-priority, time-sliced pass: the + // box updates urgently off `query`, the board keeps showing the current lens + // until the deferred render is ready, and React can interrupt a stale broad + // render when you keep typing. `stale` dims the results while it catches up. + const deferredQuery = useDeferredValue(query); + const searching = deferredQuery.trim().length > 0; + const searchStale = query !== deferredQuery; // narrows the board to one or more review-effort classes ('xs'..'xl', // 'unknown'); empty = no filter const [weightSel, setWeightSel] = useState(() => urlState.weight); @@ -428,16 +453,22 @@ export function App() { scope.authors.includes(login) || queryAuthors.some(q => login.toLowerCase().includes(q)), [scope.authors, queryAuthors] ); + // "is:bot" is a blanket reveal (it doesn't name a specific bot the way + // author: does), so it's a query-level flag rather than another + // per-login predicate like revealedAuthor. + const queryHasBotToken = useMemo( + () => query.toLowerCase().split(/\s+/).includes('is:bot'), + [query] + ); - // The one is-this-pull-off-the-board predicate: hidden repos, hidden - // people, cryo, snoozes, and the drafts rule stay off unless a session - // act reveals them — an explicit reveal, a scope, a repo:/author: query - // term, or the master "show everything". Never hides your own pulls; bots - // hide only when you turn on Ignore bot PRs (they otherwise have their own - // fold), and a person can't hide themselves. Shared by the filter pipeline and the - // hidden-PR ledger's counts so the ledger's number can never disagree - // with what the board actually withholds. - const boardHidden = useCallback( + // Every off-by-default rule EXCEPT "Ignore bot PRs": hidden repos/people, + // parked (cryo) pulls, and the drafts-mine default. Split out of + // boardHidden below so botsBypassingHideBots (further down, feeding the CI + // lens and Ready-to-merge) can reuse it for a bot pool that skips ONLY the + // hideBots rule — those two surfaces need bot signal (fleet build health, + // a merge-ready bot PR) even when a reviewer has muted bots from Review's + // human-review surfaces. + const boardHiddenExceptBots = useCallback( (p: DerivedPull) => { if (showAll) return false; const hidden = isHiddenFor( @@ -459,15 +490,7 @@ export function App() { p.data.user.login !== me && !revealedAuthor(p.data.user.login) && !reviewRequestedFrom(p, me); - // snoozes are NOT here: a snooze only quiets the Review lens (its - // "not today" gesture), so the Review view applies it itself and - // every other lens still shows the pull - // "Ignore bot PRs": a saved preference that keeps machine authors off - // the board entirely (they otherwise live in their own low-priority - // fold). "Show everything" still reveals them via the showAll early - // return above. - const botHidden = settings.hideBots && isBot(p); - return hidden || cryoHidden || draftHidden || botHidden; + return hidden || cryoHidden || draftHidden; }, [ showAll, @@ -479,11 +502,46 @@ export function App() { settings.repoPrefs, settings.showCryo, settings.hiddenPeople, - settings.hideBots, draftsMode, ] ); + // The one is-this-pull-off-the-board predicate: hidden repos, hidden + // people, cryo, snoozes, and the drafts rule stay off unless a session + // act reveals them — an explicit reveal, a scope, a repo:/author: query + // term, or the master "show everything". Never hides your own pulls; bots + // hide only when you turn on Ignore bot PRs (they otherwise have their own + // fold), and a person can't hide themselves. Shared by the filter pipeline and the + // hidden-PR ledger's counts so the ledger's number matches this predicate + // exactly — with one deliberate exception: the CI lens and Ready-to-merge + // read botsBypassingHideBots (below) instead of this function, so a bot + // PR the ledger counts as hidden by "Ignore bot PRs" can still show there. + const boardHidden = useCallback( + (p: DerivedPull) => { + if (showAll) return false; + // snoozes are NOT here: a snooze only quiets the Review lens (its + // "not today" gesture), so the Review view applies it itself and + // every other lens still shows the pull + // "Ignore bot PRs": a saved preference that keeps machine authors off + // Review's human-review surfaces (they otherwise live in their own + // low-priority fold and the queue tail). A bot named by the active + // saved/pinned filter (revealedAuthor, via scope.authors carrying its + // login) or a typed author: / is:bot query term still shows — + // the same reveal story every other hidden rule gets. "Show + // everything" still reveals them via the showAll early return above. + // The CI lens and Ready-to-merge bypass this rule entirely, reveal or + // not (see botsBypassingHideBots below): fleet build health and a + // merge-ready bot PR matter regardless of this setting. + const botHidden = + settings.hideBots && + isBot(p) && + !revealedAuthor(p.data.user.login) && + !queryHasBotToken; + return boardHiddenExceptBots(p) || botHidden; + }, + [showAll, boardHiddenExceptBots, settings.hideBots, isBot, revealedAuthor, queryHasBotToken] + ); + // Nudges/notifications honor the STANDING hidden-repo/hidden-person // settings only — never the current view's scope/query/weight/state // filters, and never a session reveal — so a hidden-repo (or hidden- @@ -508,10 +566,13 @@ export function App() { // settings above), not the current filter useNotifications(nudgeablePulls, me, turns, initialized); - // every existing scope/query pass, but not yet the Weight/State filters — - // WeightFilter's live per-option counts read this pool, so narrowing to - // one weight class doesn't make the other classes' counts vanish (the - // same reasoning PeopleFilter's authorCounts follows for its own pool) + // scope + hidden, but deliberately NOT free text: a query drives the global + // Search lens (App renders instead of the current lens), so + // filtering the whole board by it here would recompute every keystroke only + // to feed a view that never shows while searching. Also not the Weight/State + // filters yet: WeightFilter's live per-option counts read this pool, so + // narrowing to one weight class doesn't make the other classes' counts + // vanish (the same reasoning PeopleFilter's authorCounts follows). const preWeightScoped = useMemo(() => { let out = pulls.filter(p => !boardHidden(p)); if (scope.repos.length) out = out.filter(p => scope.repos.includes(p.data.repo)); @@ -523,9 +584,8 @@ export function App() { // allow-list above (a dependency bump is nobody's teammate) if (scope.notAuthors.length) out = out.filter(p => isBot(p) || !scope.notAuthors.includes(p.data.user.login)); - if (query) out = out.filter(p => matchesQuery(p, query, me, names)); return out; - }, [pulls, boardHidden, scope, isBot, query, names, me]); + }, [pulls, boardHidden, scope, isBot]); // preWeightScoped narrowed by the Weight filter, but not yet State — // StateFilter's own live counts read this pool for the same @@ -550,6 +610,32 @@ export function App() { // (each keystroke, settings toggle, and heartbeat re-runs App) const humans = useMemo(() => scoped.filter(p => !isBot(p)), [scoped, isBot]); const bots = useMemo(() => scoped.filter(isBot), [scoped, isBot]); + + // Bots that pass every hidden rule except "Ignore bot PRs": hidden + // repos/people, cryo, and the drafts rule still apply, and the active + // repo scope / weight / state filters narrow it the same way they narrow + // `scoped` (scope.authors/notAuthors don't need repeating here — bots + // already bypass both in preWeightScoped above). Free text is left out for + // the same reason as preWeightScoped: a query shows the Search lens, not + // this pool's CI/Ready surfaces. Only the hideBots-specific exclusion is + // skipped. Feeds the CI lens and Ready-to-merge (below), the two surfaces + // that need bot signal regardless of hideBots. + const botsBypassingHideBots = useMemo(() => { + let out = pulls.filter(p => isBot(p) && !boardHiddenExceptBots(p)); + if (scope.repos.length) out = out.filter(p => scope.repos.includes(p.data.repo)); + if (weightSel.length) out = out.filter(p => matchesWeightFilter(p, weightSel)); + if (stateSel.length) out = out.filter(p => stateSel.includes(actionState(p, me))); + return out; + }, [pulls, isBot, boardHiddenExceptBots, scope.repos, me, weightSel, stateSel]); + // The CI lens's pool: `scoped` (which already respects hideBots, escape + // hatch included) plus whatever bot the bypass pool above adds back — + // deduped by key so a bot already visible in `scoped` (hideBots off, or + // individually revealed) is never doubled. + const ciPulls = useMemo(() => { + const scopedKeys = new Set(scoped.map(p => pullKey(p.data))); + const extraBots = botsBypassingHideBots.filter(p => !scopedKeys.has(pullKey(p.data))); + return extraBots.length ? [...scoped, ...extraBots] : scoped; + }, [scoped, botsBypassingHideBots]); // every known repo with its open-PR count — feeds the Filters popover and // the Settings repo manager. Includes pref'd repos at 0. const repoCounts = useMemo(() => { @@ -614,7 +700,9 @@ export function App() { return c; }, [pulls, me, settings.repoPrefs, settings.hiddenPeople, isBot, boardHidden]); - const isScoped = scope.repos.length || scope.authors.length || query; + // a query no longer scopes the board (it shows the Search lens instead), so + // the header's "X of Y" only reflects the repo/people filters + const isScoped = scope.repos.length || scope.authors.length; // what a "Save current filter…" click right now would capture — reads the // same hashState the write-effect above builds, so a saved filter's hash @@ -640,10 +728,17 @@ export function App() { // an avatar click anywhere lands on that person's board: their login // becomes the authors scope (visible in the bar, clearable there too) + // a user picking a lens leaves search: clear the query so the chosen lens + // actually renders (a lingering query keeps the Search lens up) + const goToLens = useCallback((id: Lens) => { + setQuery(''); + setLens(id); + }, []); // and the Team lens shows the result const onPerson = useCallback( (login: string) => { setScope({ ...scope, authors: [login], notAuthors: [] }); + setQuery(''); setLens('team'); }, [scope, setScope] @@ -700,11 +795,13 @@ export function App() { const tab = (id: Lens, label: string, count?: number) => ( - ); - })} + {/* Pinned saved searches get their own line. Reviewers deliberately + pin many (every roster earns one automatically, plus hand-saved + ones), which overflowed the shared trigger row into a wall. A + dedicated horizontally-scrollable strip keeps every pin one click + away and stops a growing set from wrapping the dimension triggers + below. Click applies it on the lens you're standing on; click + again clears. State is a background tint; the text never changes, + so nothing shifts. */} + {pinnedSearches.length > 0 && ( +
+ {pinnedSearches.map(f => { + const active = matchesView(f.hash, currentHash); + const title = active + ? `showing ${f.name}: click to clear` + : `show ${f.name}: ${describeHash(f.hash)}`; + return ( + + ); + })} +
+ )} +
0 ? '' : 'border-t border-secondary' + }`} + > @@ -1056,18 +1180,33 @@ export function App() { Loading the board…
)} - {initialized && lens === 'review' && ( + {/* free text is a find, not a filter: while there's a query the + board becomes the global Search lens (open + closed, every lens, + nothing folded), regardless of which tab was active */} + {initialized && searching && ( + + )} + {initialized && !searching && lens === 'review' && ( )} - {initialized && lens === 'mine' && ( + {initialized && !searching && lens === 'mine' && ( )} - {initialized && lens === 'team' && ( + {initialized && !searching && lens === 'team' && ( !isBot(p))} @@ -1077,9 +1216,11 @@ export function App() { extraBots={extraBots} /> )} - {initialized && lens === 'classic' && } - {initialized && lens === 'ci' && } - {initialized && lens === 'stats' && ( + {initialized && !searching && lens === 'classic' && ( + + )} + {initialized && !searching && lens === 'ci' && } + {initialized && !searching && lens === 'stats' && ( )} diff --git a/frontend-v2/src/components/NotificationPanel.tsx b/frontend-v2/src/components/NotificationPanel.tsx index 45a4da87..57a673be 100644 --- a/frontend-v2/src/components/NotificationPanel.tsx +++ b/frontend-v2/src/components/NotificationPanel.tsx @@ -1,6 +1,6 @@ import { useState } from 'react'; import { ArrowLeft, Bell, Settings as SettingsIcon, X } from 'lucide-react'; -import { ago, githubUrl } from '../../../shared/format'; +import { ago, githubUrl, pullKey, shortRepo } from '../../../shared/format'; import { type CheerGroup, CONFIGURABLE_TOASTS } from '../model/cheers'; import { type Settings as SettingsShape, @@ -15,6 +15,7 @@ import { testNotification, unlockSound, } from '../notifications'; +import { TOAST_PULLS_SHOWN } from '../model/toast'; import type { ToastRecord } from '../toasts'; import { HeaderIconButton, QuietButton, Segmented, Switch } from './bits'; import { Icon } from './Icon'; @@ -361,23 +362,46 @@ export function NotificationPanel({ - + + {r.toast.count != null && `${r.toast.count} `} {r.toast.title} {ago(r.at / 1000)} - {r.toast.pull && ( - - #{r.toast.pull.number} - {r.toast.pull.title ? ` ${r.toast.pull.title}` : ''} - + {r.toast.pulls && r.toast.pulls.length > 0 ? ( + + {r.toast.pulls.slice(0, TOAST_PULLS_SHOWN).map(p => ( + + {shortRepo(p.repo)}#{p.number} + {p.title ? ` ${p.title}` : ''} + + ))} + {r.toast.pulls.length > TOAST_PULLS_SHOWN && ( + + +{r.toast.pulls.length - TOAST_PULLS_SHOWN} more + + )} + + ) : ( + r.toast.pull && ( + + {shortRepo(r.toast.pull.repo)}#{r.toast.pull.number} + {r.toast.pull.title ? ` ${r.toast.pull.title}` : ''} + + ) )} {r.toast.body && ( {r.toast.body} diff --git a/frontend-v2/src/components/Popover.tsx b/frontend-v2/src/components/Popover.tsx index e2ded01e..45dc0474 100644 --- a/frontend-v2/src/components/Popover.tsx +++ b/frontend-v2/src/components/Popover.tsx @@ -1,4 +1,10 @@ -import { useLayoutEffect, useState, type ReactNode, type Ref } from 'react'; +import { + type PointerEvent as ReactPointerEvent, + useLayoutEffect, + useState, + type ReactNode, + type Ref, +} from 'react'; import { createPortal } from 'react-dom'; import { usePopover } from './usePopover'; @@ -8,9 +14,10 @@ export interface PopoverTriggerProps { 'aria-expanded': boolean; onClick: () => void; /** present only with `hoverTriggerOnly`: the hover handlers move off the - * root and onto the trigger element itself */ - onMouseEnter?: () => void; - onMouseLeave?: () => void; + * root and onto the trigger element itself. Pointer events, not mouse, so + * the hover-arm can be gated to a real mouse and skip touch taps. */ + onPointerEnter?: (e: ReactPointerEvent) => void; + onPointerLeave?: (e: ReactPointerEvent) => void; } /** diff --git a/frontend-v2/src/components/Row.tsx b/frontend-v2/src/components/Row.tsx index 8130ef09..985297cf 100644 --- a/frontend-v2/src/components/Row.tsx +++ b/frontend-v2/src/components/Row.tsx @@ -280,7 +280,7 @@ function useRowActions(pull: DerivedPull) { setCopied(true); setTimeout(() => setCopied(false), 1200); }, - snooze: () => snoozePull(pullKey(pull.data)), + snooze: () => snoozePull(pull.data), unsnooze: () => unsnoozePull(pullKey(pull.data)), refresh: () => { refreshPull(pull.data.repo, pull.data.number); diff --git a/frontend-v2/src/components/bits.tsx b/frontend-v2/src/components/bits.tsx index d64c7d30..d0e2f7ff 100644 --- a/frontend-v2/src/components/bits.tsx +++ b/frontend-v2/src/components/bits.tsx @@ -401,20 +401,60 @@ export function Switch({ ); } -export function EmptyState({ title, sub }: { title: string; sub: string }) { +export function EmptyState({ + title, + sub, + variant = 'ok', +}: { + title: string; + sub: string; + /** 'ok' is the green check: a genuine all-clear, nothing left to do (a good + * state). 'search' is the muted magnifying glass, for a null result from + * narrowing the board (a search or filter matched nothing) — where the check + * would wrongly read as success. Both draw the same stroke-in. */ + variant?: 'ok' | 'search'; +}) { return (
- - - - + > + + + + + + + ) : ( + + + + + )} {title} {sub}
diff --git a/frontend-v2/src/components/usePopover.ts b/frontend-v2/src/components/usePopover.ts index 89c64186..c8cef9fd 100644 --- a/frontend-v2/src/components/usePopover.ts +++ b/frontend-v2/src/components/usePopover.ts @@ -1,4 +1,4 @@ -import { useEffect, useRef, useState } from 'react'; +import { type PointerEvent as ReactPointerEvent, useEffect, useRef, useState } from 'react'; import { getSettings } from '../settings'; /** @@ -52,8 +52,18 @@ export function usePopover { - if (!opts?.hover) return; + // Only touch is excluded from hover. Touch has no hover: a tap + // synthesizes pointerenter, which used to arm the open timer and fire the + // preview ~250ms LATER, after the tap's real action (a link navigation, a + // filter) had already run — an uninvited popover on top of the page you + // were just sent to. Gating on pointerType === 'touch' (rather than + // requiring 'mouse') keeps desktop hover intact, handles hybrid + // mouse+touch devices per interaction unlike a device-level (hover: hover) + // check, and lets a stylus ('pen') and any other non-touch pointer keep + // hover too. Button doors still open on tap via toggle() (a click), so + // touch keeps every tap-to-open popover. + const onPointerEnter = (e: ReactPointerEvent) => { + if (!opts?.hover || e.pointerType === 'touch') return; clearClose(); // a short intent delay (a user setting): brushing the cursor across a row // of triggers shouldn't flash their panels open one after another. Read @@ -64,8 +74,8 @@ export function usePopover setOpen(true), delay); }; - const onMouseLeave = () => { - if (!opts?.hover) return; + const onPointerLeave = (e: ReactPointerEvent) => { + if (!opts?.hover || e.pointerType === 'touch') return; clearOpen(); if (pinned.current) return; clearClose(); @@ -117,6 +127,6 @@ export function usePopover { note({ status: 'needs_cr', changesRequestedBy: ['carol'], headPushedAt: 200 }, me) ).toEqual({ action: 'Address feedback', context: 'changes requested by carol' }); }); + + it('a bot CHANGES_REQUESTED date never skews whether the author answered a human', () => { + // carol (human) requested changes at t=100; the author pushed at + // t=200, answering her. claude[bot] also left a later + // CHANGES_REQUESTED-flavored review at t=900 -- its date must not make + // carol's request look unanswered, or reset "Address feedback" over a + // human reviewer who's already been pushed past. + expect( + note( + { + status: 'needs_cr', + changesRequestedBy: ['carol'], + unstampedReviewers: [ + { login: 'carol', state: 'CHANGES_REQUESTED', date: 100 }, + { login: 'claude[bot]', state: 'CHANGES_REQUESTED', date: 900 }, + ], + headPushedAt: 200, + }, + me + ) + ).toEqual({ + action: null, + context: `waiting on carol to re-review · last commit ${ago(200)} ago`, + }); + }); }); describe('parked (Cryogenic Storage) — asks nothing of anyone', () => { diff --git a/frontend-v2/src/model/actions.ts b/frontend-v2/src/model/actions.ts index dade8dc8..7aec4a77 100644 --- a/frontend-v2/src/model/actions.ts +++ b/frontend-v2/src/model/actions.ts @@ -211,7 +211,7 @@ function authorNote(p: DerivedPull, me: string): RowNote { * send them). */ function lastChangesRequestedAt(p: DerivedPull): number | null { const dates = (p.data.status.unstamped_reviewers ?? []) - .filter(r => r.state === 'CHANGES_REQUESTED') + .filter(r => r.state === 'CHANGES_REQUESTED' && !isSuffixBot(r.login)) .map(r => r.date); return dates.length ? Math.max(...dates) : null; } diff --git a/frontend-v2/src/model/notificationRelevance.test.ts b/frontend-v2/src/model/notificationRelevance.test.ts new file mode 100644 index 00000000..81474695 --- /dev/null +++ b/frontend-v2/src/model/notificationRelevance.test.ts @@ -0,0 +1,51 @@ +import { describe, expect, it } from 'vitest'; +import { SHIPPED_TOAST_KIND } from './cheers'; +import { notificationStale } from './notificationRelevance'; +import type { Toast } from './toast'; + +/** A toast carrying only the fields the relevance check reads. */ +function toast(o: Partial): Toast { + return { tone: 'info', icon: '🤝', title: 't', ...o }; +} + +const ref = (repo: string, number: number) => ({ repo, number }); + +describe('notificationStale', () => { + const open = new Set(['fixbot#3116', 'org/repo#7']); + + it('drops a single-pull nudge once its PR leaves the open board', () => { + // Kyle's report: "return the favor on fixbot#3116" after #3116 merged + const merged = toast({ kind: 'return-the-favor', pull: ref('fixbot', 3116) }); + expect(notificationStale(merged, new Set(['org/repo#7']))).toBe(true); + // while the PR is still open, the nudge stays + expect(notificationStale(merged, open)).toBe(false); + }); + + it('keeps a toast that points at no PR (board-wide rewards)', () => { + const inboxZero = toast({ kind: 'inbox-zero', pull: undefined }); + expect(notificationStale(inboxZero, new Set())).toBe(false); + }); + + it('never stales the retrospective shipped recap, even though its PRs are merged', () => { + const shipped = toast({ + kind: SHIPPED_TOAST_KIND, + pulls: [ref('org/repo', 900), ref('org/repo', 901)], + }); + expect(notificationStale(shipped, new Set())).toBe(false); + }); + + it('stales a batched toast only once every referenced pull is gone', () => { + const batch = toast({ pulls: [ref('org/repo', 7), ref('fixbot', 3116)] }); + // one of the two still open -> still relevant + expect(notificationStale(batch, new Set(['org/repo#7']))).toBe(false); + // both gone -> stale + expect(notificationStale(batch, new Set())).toBe(true); + }); + + it('applies to every PR-linked kind, not just return-the-favor', () => { + for (const kind of ['your-turn', 'review-requested', 'pr-ci-red', 're-stamp-owed']) { + const t = toast({ kind, pull: ref('gone', 1) }); + expect(notificationStale(t, open)).toBe(true); + } + }); +}); diff --git a/frontend-v2/src/model/notificationRelevance.ts b/frontend-v2/src/model/notificationRelevance.ts new file mode 100644 index 00000000..40a9cf8e --- /dev/null +++ b/frontend-v2/src/model/notificationRelevance.ts @@ -0,0 +1,41 @@ +import { pullKey } from '../../../shared/format'; +import { SHIPPED_TOAST_KIND } from './cheers'; +import type { Toast } from './toast'; + +/** Toast kinds that are ABOUT already-merged PRs, so a referenced pull missing + * from the open board is the whole point, not staleness. The shipped catch-up + * is the only one today; a new retrospective toast joins here. */ +const RETROSPECTIVE_KINDS: ReadonlySet = new Set([SHIPPED_TOAST_KIND]); + +/** The pull keys a toast points you at: its single `pull`, its `pulls` batch, + * or neither (a board-wide reward like inbox-zero). */ +function toastPullKeys(toast: Toast): string[] { + const refs = toast.pulls?.length ? toast.pulls : toast.pull ? [toast.pull] : []; + return refs.map(pullKey); +} + +/** + * Whether a fired notification has gone stale: the PR it points you at has left + * the open board (merged or closed), so whatever it asked you to do can't be + * done anymore. The notification panel drops these, so a nudge like "return the + * favor on fixbot#3116" removes itself the moment #3116 merges instead of + * lingering as a dead link (prod report, Kyle 2026-07-27). + * + * Deliberately general -- every PR-linked kind, not just return-the-favor -- so + * one rule governs the whole panel. Two shapes are never stale: a toast with no + * PR (inbox-zero, board-cleared, a milestone count), since there's nothing to + * resolve, and a retrospective recap (shipped), which lists merged PRs on + * purpose. A toast pointing at several pulls is stale only once every one of + * them is gone. + * + * `openKeys` is the set of pull keys currently on the OPEN board; a referenced + * key that's absent has merged or closed. Feed it the FULL board rather than + * the hidden-filtered subset, so a still-open PR you've merely hidden isn't + * mistaken for done. + */ +export function notificationStale(toast: Toast, openKeys: ReadonlySet): boolean { + if (toast.kind && RETROSPECTIVE_KINDS.has(toast.kind)) return false; + const keys = toastPullKeys(toast); + if (keys.length === 0) return false; + return keys.every(key => !openKeys.has(key)); +} diff --git a/frontend-v2/src/model/query.test.ts b/frontend-v2/src/model/query.test.ts index a15633d8..021f5bac 100644 --- a/frontend-v2/src/model/query.test.ts +++ b/frontend-v2/src/model/query.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it } from 'vitest'; import type { DerivedPull, Weight } from '../../../shared/model/status'; -import { matchesQuery } from './query'; +import type { PullData } from '../../../shared/types'; +import { matchesClosedQuery, matchesQuery } from './query'; function fake( over: Partial<{ @@ -136,6 +137,20 @@ describe('matchesQuery', () => { expect(matchesQuery(fake({ status: 'ready' }), 'is:blocked', 'viewer')).toBe(false); }); + it('is:bot matches a [bot]-suffixed author', () => { + expect(matchesQuery(fake({ author: 'dependabot[bot]' }), 'is:bot', 'viewer')).toBe(true); + expect(matchesQuery(fake({ author: 'alice' }), 'is:bot', 'viewer')).toBe(false); + }); + + it("is:bot also matches a config.json extra-bots login, given the caller's set", () => { + const extraBots = new Set(['ifixit-systems']); + expect(matchesQuery(fake({ author: 'ifixit-systems' }), 'is:bot', 'viewer', extraBots)).toBe( + true + ); + // absent the set (the default), a non-suffixed login isn't recognized as a bot + expect(matchesQuery(fake({ author: 'ifixit-systems' }), 'is:bot', 'viewer')).toBe(false); + }); + it('unknown has:/is: values match nothing', () => { expect(matchesQuery(fake({}), 'has:bogus', 'viewer')).toBe(false); expect(matchesQuery(fake({}), 'is:bogus', 'viewer')).toBe(false); @@ -150,3 +165,67 @@ describe('matchesQuery', () => { expect(matchesQuery(wrongStatus, 'weight:xs is:blocked', 'viewer')).toBe(false); }); }); + +function fakeClosed( + over: Partial<{ + title: string; + repo: string; + author: string; + number: number; + labels: string[]; + }> +): PullData { + return { + title: over.title ?? 'Rework the offer-sync batch loop', + repo: over.repo ?? 'acme/widgets', + number: over.number ?? 63258, + user: { login: over.author ?? 'alice' }, + labels: (over.labels ?? []).map(title => ({ title })), + } as unknown as PullData; +} + +describe('matchesClosedQuery', () => { + it('matches the identity fields a find actually uses', () => { + const p = fakeClosed({ + title: 'Store offer sync', + author: 'alice', + number: 42, + labels: ['QAE'], + }); + expect(matchesClosedQuery(p, 'offer', 'viewer')).toBe(true); + expect(matchesClosedQuery(p, '42', 'viewer')).toBe(true); + expect(matchesClosedQuery(p, '#42', 'viewer')).toBe(true); + expect(matchesClosedQuery(p, 'repo:widgets', 'viewer')).toBe(true); + expect(matchesClosedQuery(p, 'author:ali', 'viewer')).toBe(true); + expect(matchesClosedQuery(p, 'label:qae', 'viewer')).toBe(true); + expect(matchesClosedQuery(p, 'offer nope', 'viewer')).toBe(false); + }); + + it('is:bot and is:mine work on a closed pull', () => { + expect(matchesClosedQuery(fakeClosed({ author: 'renovate[bot]' }), 'is:bot', 'viewer')).toBe( + true + ); + expect( + matchesClosedQuery( + fakeClosed({ author: 'ifixit-systems' }), + 'is:bot', + 'viewer', + new Set(['ifixit-systems']) + ) + ).toBe(true); + expect(matchesClosedQuery(fakeClosed({ author: 'alice' }), 'is:mine', 'alice')).toBe(true); + expect(matchesClosedQuery(fakeClosed({ author: 'alice' }), 'is:mine', 'bob')).toBe(false); + }); + + it('a state-only token never matches a closed pull (it has no live state)', () => { + // status:/weight:/older:/has:/is:restamp describe an OPEN board state, so a + // query using one shouldn't drag closed pulls into the results by accident + const p = fakeClosed({ title: 'offer sync' }); + expect(matchesClosedQuery(p, 'status:ready', 'viewer')).toBe(false); + expect(matchesClosedQuery(p, 'weight:xs', 'viewer')).toBe(false); + expect(matchesClosedQuery(p, 'has:action', 'viewer')).toBe(false); + expect(matchesClosedQuery(p, 'is:restamp', 'viewer')).toBe(false); + // but a bare term in the same pull still matches on its own + expect(matchesClosedQuery(p, 'offer', 'viewer')).toBe(true); + }); +}); diff --git a/frontend-v2/src/model/query.ts b/frontend-v2/src/model/query.ts index f35e853e..1284eff9 100644 --- a/frontend-v2/src/model/query.ts +++ b/frontend-v2/src/model/query.ts @@ -1,5 +1,7 @@ import { authorOwnsIt, parked, rowNote } from './actions'; import type { DerivedPull } from '../../../shared/model/status'; +import type { PullData } from '../../../shared/types'; +import { isBotLogin } from '../../../shared/model/visibility'; /** * The filter box grammar. Bare terms AND-match as substrings across title, @@ -15,26 +17,35 @@ import type { DerivedPull } from '../../../shared/model/status'; * has:action the viewer (`me`) has an imperative move on this card * is:restamp `me` owes a re-CR or re-QA on a reviewable pull * is:blocked status is dev_block or deploy_block + * is:bot the author is a bot: the `[bot]` suffix OR a login named in + * config.json's `bots` list (the caller's `extraBots` set) — + * a query-layer bot check must agree with what the board + * itself treats as a bot, or an active is:bot term narrows a + * pool (app.tsx's CI/Ready bot pool included) to fewer PRs + * than that pool actually holds * * `me` is the viewer's login, needed only for has:/is: — every other token - * ignores it. `names` is the optional login → display-name map (model/ - * names.ts): when present, author: and bare terms match the human name too, - * so "metz" finds djmetzle. + * ignores it. `extraBots` is config.json's non-suffix bot logins (empty set + * if the caller has none), needed only for is:bot. `names` is the optional + * login → display-name map (model/names.ts): when present, author: and bare + * terms match the human name too, so "metz" finds djmetzle. */ export function matchesQuery( p: DerivedPull, query: string, me: string, + extraBots: ReadonlySet = new Set(), names?: Readonly> ): boolean { const terms = query.toLowerCase().split(/\s+/).filter(Boolean); - return terms.every(t => matchTerm(p, t, me, names)); + return terms.every(t => matchTerm(p, t, me, extraBots, names)); } function matchTerm( p: DerivedPull, term: string, me: string, + extraBots: ReadonlySet, names?: Readonly> ): boolean { const d = p.data; @@ -66,6 +77,7 @@ function matchTerm( if (val === 'blocked') return p.status === 'dev_block' || p.status === 'deploy_block'; if (val === 'draft') return p.status === 'draft'; if (val === 'mine') return d.user.login.toLowerCase() === me.toLowerCase(); + if (val === 'bot') return isBotLogin(d.user.login, extraBots); // unrecognized is: value: fall through to the plain substring // match below, same as any other unknown key } @@ -82,3 +94,67 @@ function matchTerm( d.labels.some(l => l.title.toLowerCase().includes(term)) ); } + +/** + * The same grammar over a CLOSED pull (raw PullData, no derived status). The + * search lens runs this so a merged or closed PR is findable by the identity + * terms people actually search on: 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 and identity tokens match exactly as + * matchTerm does for open pulls, so "offer" finds the same fields either side of + * the open/closed line. + */ +export function matchesClosedQuery( + pd: PullData, + query: string, + me: string, + extraBots: ReadonlySet = new Set(), + names?: Readonly> +): boolean { + const terms = query.toLowerCase().split(/\s+/).filter(Boolean); + return terms.every(t => matchClosedTerm(pd, t, me, extraBots, names)); +} + +/** State-only token keys: they read a live derivation a closed pull no longer + * has, so a closed pull can never satisfy them. */ +const OPEN_ONLY_KEYS = new Set(['status', 'weight', 'older', 'has']); + +function matchClosedTerm( + pd: PullData, + term: string, + me: string, + extraBots: ReadonlySet, + names?: Readonly> +): boolean { + const authorName = names?.[pd.user.login]?.toLowerCase() ?? ''; + const i = term.indexOf(':'); + if (i > 0) { + const key = term.slice(0, i); + const val = term.slice(i + 1); + if (val) { + if (key === 'repo') return pd.repo.toLowerCase().includes(val); + if (key === 'author') + return pd.user.login.toLowerCase().includes(val) || authorName.includes(val); + if (key === 'label') return pd.labels.some(l => l.title.toLowerCase().includes(val)); + if (key === 'is') { + if (val === 'bot') return isBotLogin(pd.user.login, extraBots); + if (val === 'mine') return pd.user.login.toLowerCase() === me.toLowerCase(); + // is:restamp/blocked/draft describe an open pull's state + return false; + } + if (OPEN_ONLY_KEYS.has(key)) return false; + // unknown key: fall through to the substring match below + } + } + const bare = term.startsWith('#') ? term.slice(1) : term; + if (/^\d+$/.test(bare)) return String(pd.number).includes(bare); + return ( + pd.title.toLowerCase().includes(term) || + pd.repo.toLowerCase().includes(term) || + pd.user.login.toLowerCase().includes(term) || + (authorName !== '' && authorName.includes(term)) || + pd.labels.some(l => l.title.toLowerCase().includes(term)) + ); +} diff --git a/frontend-v2/src/model/repoBlocks.test.ts b/frontend-v2/src/model/repoBlocks.test.ts index 65a712bf..edc5c355 100644 --- a/frontend-v2/src/model/repoBlocks.test.ts +++ b/frontend-v2/src/model/repoBlocks.test.ts @@ -27,13 +27,13 @@ describe('repoBlocks', () => { expect(shape(repoBlocks(queue, ['ops'])).blocks[0].nums).toEqual([9, 1, 5]); }); - it('starved pulls pierce the partition and lead in their own order', () => { + it('surfaces starved pulls as a highlight while keeping them in their own repo block', () => { const queue = [pull('mono', 1), pull('ops', 2, true), pull('mono', 3, true), pull('ops', 4)]; expect(shape(repoBlocks(queue, ['mono', 'ops']))).toEqual({ starved: [2, 3], blocks: [ - { repo: 'mono', nums: [1] }, - { repo: 'ops', nums: [4] }, + { repo: 'mono', nums: [1, 3] }, + { repo: 'ops', nums: [2, 4] }, ], }); }); diff --git a/frontend-v2/src/model/repoBlocks.ts b/frontend-v2/src/model/repoBlocks.ts index ca859523..13b1470d 100644 --- a/frontend-v2/src/model/repoBlocks.ts +++ b/frontend-v2/src/model/repoBlocks.ts @@ -10,9 +10,12 @@ import type { DerivedPull } from '../../../shared/model/status'; * monopolize the screen). * * Two guarantees survive the partition: - * - Starved pulls pierce it. They lead the lane regardless of repo, in their - * existing rank order — the fairness backstop must not sit below a repo - * the viewer ranked last, or "surface the other repos" becomes a lie. + * - Starved pulls are surfaced as a highlight (returned separately, still in + * rank order) AND kept in their own repo block — a starved pull from a + * low-priority repo is never hidden by a higher-priority repo's block, + * unlike an earlier version that extracted starved pulls out of the + * blocks entirely and could bury one behind a high-volume repo's "show + * more" fold. * - Order WITHIN a block is exactly the incoming order (stable partition), * so teammates still lead and the score still ranks — the priority only * decides which repo's run comes first, never which pull is "best". @@ -31,9 +34,8 @@ export function repoBlocks( priority: string[] ): { starved: DerivedPull[]; blocks: RepoBlock[] } { const starved = queue.filter(p => p.starved); - const rest = queue.filter(p => !p.starved); const byRepo = new Map(); - for (const p of rest) { + for (const p of queue) { const list = byRepo.get(p.data.repo); if (list) list.push(p); else byRepo.set(p.data.repo, [p]); diff --git a/frontend-v2/src/model/reviewLanes.test.ts b/frontend-v2/src/model/reviewLanes.test.ts index 265227ff..1729234f 100644 --- a/frontend-v2/src/model/reviewLanes.test.ts +++ b/frontend-v2/src/model/reviewLanes.test.ts @@ -260,11 +260,11 @@ describe('buildReviewLanes — waiting on you vs waiting on others', () => { }); describe('buildReviewLanes — region matches', () => { - it('surfaces a pull matching a configured code region and pulls it out of the queue', () => { + it('surfaces a pull matching a configured code region as a highlight, without removing it from the queue', () => { const p = dp({ author: 'alice', status: 'needs_cr', title: 'Rework the Shopify sync' }); const lanes = buildReviewLanes(input({ pulls: [p], codeRegions: ['Shopify'] })); expect(lanes.regionMatches).toEqual([p]); - expect(lanes.queue).not.toContain(p); + expect(lanes.queue).toContain(p); }); it('leaves regionMatches empty when no regions are configured', () => { @@ -274,7 +274,7 @@ describe('buildReviewLanes — region matches', () => { expect(lanes.queue).toEqual([p]); }); - it('pulls a QA-pool match out of needsQa into regionMatches too', () => { + it('surfaces a QA-pool match in regionMatches too, without removing it from needsQa', () => { const p = dp({ author: 'alice', status: 'needs_qa', @@ -282,7 +282,40 @@ describe('buildReviewLanes — region matches', () => { }); const lanes = buildReviewLanes(input({ pulls: [p], codeRegions: ['Shopify'] })); expect(lanes.regionMatches).toEqual([p]); - expect(lanes.needsQa).not.toContain(p); + expect(lanes.needsQa).toContain(p); + }); + + it('excludes a pull you already CR-stamped from regionMatches, even via the QA pool', () => { + // qaPool itself doesn't care about crBy — a needs_qa pull with a live + // stamp from you would otherwise sail through the QA-pool leg and land + // in "your code regions" as if it were news to you. It still belongs + // in needsQa (that lane's own filters don't look at crBy at all). + const p = dp({ + author: 'alice', + status: 'needs_qa', + crBy: ['me'], + title: 'Rework the Shopify sync', + }); + const lanes = buildReviewLanes(input({ pulls: [p], codeRegions: ['Shopify'] })); + expect(lanes.regionMatches).not.toContain(p); + expect(lanes.needsQa).toContain(p); + }); + + it('keeps a needs_recr pull in regionMatches when only your own stamp went stale', () => { + // recrBy including you means YOUR re-stamp is owed (a "Re-stamp" verb + // in Waiting on you) and crBy no longer includes you, since a stale + // stamp drops out of the active crBy set. The new qaPool exclusion + // only checks crBy (a currently-live stamp), so this pull still + // matches through the QA-pool leg. + const p = dp({ + author: 'alice', + status: 'needs_recr', + recrBy: ['me'], + crBy: [], + title: 'Rework the Shopify sync', + }); + const lanes = buildReviewLanes(input({ pulls: [p], codeRegions: ['Shopify'] })); + expect(lanes.regionMatches).toContain(p); }); }); @@ -300,6 +333,21 @@ describe('buildReviewLanes — stamped and ready lanes', () => { expect(lanes.ready).toEqual([bot, human]); }); + it('draws ready’s bot portion from botsForReady, not bots, when the two differ', () => { + // "Ignore bot PRs" empties `bots` (the queue-tail/fold pool) but + // botsForReady bypasses that setting — Ready-to-merge must still + // surface the bot PR even though it's absent from `bots`. + const bot = dp({ author: 'dependabot[bot]', status: 'ready' }); + const lanes = buildReviewLanes(input({ pulls: [], bots: [], botsForReady: [bot] })); + expect(lanes.ready).toEqual([bot]); + }); + + it('falls back to bots for ready when botsForReady is omitted', () => { + const bot = dp({ author: 'dependabot[bot]', status: 'ready' }); + const lanes = buildReviewLanes(input({ pulls: [], bots: [bot] })); + expect(lanes.ready).toEqual([bot]); + }); + it('keeps a stamped (CR-incomplete, your stamp live) pull out of the queue and in yoursWaiting', () => { const p = dp({ author: 'alice', status: 'needs_cr', crBy: ['me'], crReq: 2 }); const lanes = buildReviewLanes(input({ pulls: [p] })); @@ -319,6 +367,16 @@ describe('buildReviewLanes — board summary', () => { expect(lanes.empty).toBe(false); }); + it('is not empty when botsForReady holds a ready bot, even with pulls/bots/closed all empty', () => { + // the bug: "Ignore bot PRs" empties `bots`, and empty only checked + // pulls/bots/closed — a merge-ready bot reachable through botsForReady + // (and showing in lanes.ready) still read as "All clear" + const bot = dp({ author: 'dependabot[bot]', status: 'ready' }); + const lanes = buildReviewLanes(input({ botsForReady: [bot] })); + expect(lanes.empty).toBe(false); + expect(lanes.ready).toContain(bot); + }); + it('boardIsQuiet is false once the queue has something in it', () => { const p = dp({ author: 'alice', status: 'needs_cr' }); const lanes = buildReviewLanes(input({ pulls: [p] })); diff --git a/frontend-v2/src/model/reviewLanes.ts b/frontend-v2/src/model/reviewLanes.ts index e32c2482..95e17630 100644 --- a/frontend-v2/src/model/reviewLanes.ts +++ b/frontend-v2/src/model/reviewLanes.ts @@ -32,8 +32,18 @@ export interface ReviewLanesInput { * against the store's `snoozed` map) has already run, so a snoozed pull * never reaches the lane math below. */ pulls: DerivedPull[]; - /** same pool, bot pulls (dependency bumps etc.) — snoozed already excluded */ + /** same pool, bot pulls (dependency bumps etc.) — snoozed already excluded. + * This pool empties out when "Ignore bot PRs" hides bots from Review's + * human-review surfaces (the queue tail, the bot fold), unless a filter + * reveals them. */ bots: DerivedPull[]; + /** the bot pool Ready-to-merge draws from instead of `bots` — bypasses + * "Ignore bot PRs" (a green, signed-off bot PR is one merge press from + * done regardless of that setting), while still respecting every other + * hide (hidden repo/person, cryo, drafts). Defaults to `bots` so a + * caller that doesn't separate the two pools sees today's behavior: + * hiding bots hides them from Ready too. */ + botsForReady?: DerivedPull[]; /** merged/closed pulls in the loaded window — only `.length` feeds the * lanes below (the "recently closed" fold's count, the empty-board check); * Review.tsx still renders the array itself, straight from its own prop. */ @@ -92,8 +102,9 @@ export interface ReviewLanes { /** the review queue's contiguous per-repo blocks (only meaningful with * repoPriority set) */ queueBlocks: RepoBlock[]; - /** CR- or QA-pool pulls matching a configured code region, deduped and - * pulled out of every lane below */ + /** CR- or QA-pool pulls matching a configured code region, deduped — a + * highlight copy, not an extraction: matches also stay in the queue/QA + * lanes below */ regionMatches: DerivedPull[]; needsQa: DerivedPull[]; needsQaOther: DerivedPull[]; @@ -139,6 +150,7 @@ export function buildReviewLanes(input: ReviewLanesInput): ReviewLanes { const { pulls, bots, + botsForReady = bots, closed, napping, changed, @@ -259,20 +271,23 @@ export function buildReviewLanes(input: ReviewLanesInput): ReviewLanes { ); // In your code regions: reviewable pulls (CR or QA pool) matching a region - // you set in Settings, deduped across the two pools and genuinely pulled - // out of the queue/QA lanes below (including their "other repos" folds) - // into their own section — the most explicit "this is my area" signal - // earns its own spot instead of a float within the queue. + // you set in Settings, deduped across the two pools, surfaced as a + // highlight above the queue/QA lanes below — a copy of a slice, not an + // extraction (the same non-exclusive relationship "Recently updated" and + // a claimed PR already have with their lanes): a region match still shows + // up in its normal queue/QA spot too. The QA-pool leg excludes a pull you + // already hold a live CR stamp on — you've already reviewed it, so it + // isn't region news to you, even though qaPool itself doesn't otherwise + // care about CR state. const regionSeen = new Set(); const regionMatches = crSort( - [...crPool, ...qaPool].filter(p => { + [...crPool, ...qaPool.filter(p => !p.crBy.includes(me))].filter(p => { const k = pullKey(p.data); if (regionSeen.has(k) || !matchesRegion(p, codeRegions)) return false; regionSeen.add(k); return true; }) ); - const regionKeys = new Set(regionMatches.map(p => pullKey(p.data))); // ONE review queue: your primary repos' reviewables, every starved pull // regardless of repo (the fairness backstop rides in the ranking now, not @@ -280,43 +295,39 @@ export function buildReviewLanes(input: ReviewLanesInput): ReviewLanes { // them to the top numerically instead of positionally), and the day's bot // bumps sinking to the tail. The top of this lane IS the board's best // next pickup; the retired "Deal me one" button dealt this exact order, - // which is why the button became redundant and was removed. Non-starved work outside your primary repos still - // folds into "other repos" below, reachable but not in the way. Region - // matches are excluded here too (same Set-filter pattern as botKeys) — they - // live in their own lane above, not doubled up in the queue. + // which is why the button became redundant and was removed. Non-starved + // work outside your primary repos still folds into "other repos" below, + // reachable but not in the way. Region matches stay in this queue too: + // "In your code regions" above is a highlight, a copy of a slice, not an + // extraction — the same non-exclusive relationship "Recently updated" and + // a claimed PR already have with their lanes. const queue = teamFirst( dealRank( [ - ...nonStarved.filter( - p => isPrimaryRepo(p.data.repo) && !regionKeys.has(pullKey(p.data)) - ), - ...crPool.filter(p => p.starved && !regionKeys.has(pullKey(p.data))), + ...nonStarved.filter(p => isPrimaryRepo(p.data.repo)), + ...crPool.filter(p => p.starved), ...botReviewable, ], { me, pulls, deprioritize: isDemoted, warnDays: ageWarnDays } ), team ); - const queueOther = crSort( - nonStarved.filter(p => !isPrimaryRepo(p.data.repo) && !regionKeys.has(pullKey(p.data))) - ); + const queueOther = crSort(nonStarved.filter(p => !isPrimaryRepo(p.data.repo))); // Ready to merge is finishable work for anyone: fully signed off, green, // one button-press from done. It earns a real lane in Pick up next rather // than a fold — and bot PRs that reach ready join it, since a human has to // land them. Longest-waiting first: the ones most likely forgotten. + // botsForReady (not `bots`) so this lane keeps showing merge-ready bot + // PRs even when "Ignore bot PRs" has emptied `bots` out of the queue + // tail and the bot fold below. const ready = [ ...others.filter(p => p.status === 'ready'), - ...bots.filter(p => p.status === 'ready'), + ...botsForReady.filter(p => p.status === 'ready'), ].sort((a, b) => b.ageDays - a.ageDays); - const needsQa = teamFirst( - qaSort(qaPool.filter(p => isPrimaryRepo(p.data.repo) && !regionKeys.has(pullKey(p.data)))), - team - ); - const needsQaOther = qaSort( - qaPool.filter(p => !isPrimaryRepo(p.data.repo) && !regionKeys.has(pullKey(p.data))) - ); + const needsQa = teamFirst(qaSort(qaPool.filter(p => isPrimaryRepo(p.data.repo))), team); + const needsQaOther = qaSort(qaPool.filter(p => !isPrimaryRepo(p.data.repo))); // your live CR stamp is in, the PR just isn't fully signed off yet (another // reviewer owes a stamp, or a re-CR). Covers needs_recr too, so a PR you @@ -445,8 +456,12 @@ export function buildReviewLanes(input: ReviewLanesInput): ReviewLanes { botRest.length + closed.length; - // bots/shipped stay reachable even when no human PRs need review - const empty = !pulls.length && !bots.length && !closed.length; + // bots/shipped stay reachable even when no human PRs need review. Checks + // botsForReady too: a merge-ready bot can sit in `ready` via botsForReady + // while `bots` itself is empty ("Ignore bot PRs" hid it from the queue/ + // fold), and that bot must not be declared "nothing to see" out from + // under it. + const empty = !pulls.length && !bots.length && !closed.length && !botsForReady.length; return { yourMove, diff --git a/frontend-v2/src/model/shipped.test.ts b/frontend-v2/src/model/shipped.test.ts index 3836a4b6..0a1645db 100644 --- a/frontend-v2/src/model/shipped.test.ts +++ b/frontend-v2/src/model/shipped.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it } from 'vitest'; import type { PullData } from '../../../shared/types'; import { rankShipped, shipRelevance, shippedToast } from './shipped'; +import { TOAST_PULLS_SHOWN } from './toast'; /** A closed PullData carrying only the fields shipped ranking reads. */ function dp(o: { @@ -116,6 +117,23 @@ describe('shippedToast', () => { expect(toast?.body).toContain('2 you reviewed'); }); + it('lists the shipped pulls ranked, for the renderer to show a top few plus an overflow count', () => { + const shipped = [1, 2, 3, 4, 5].map(number => + dp({ number, author: 'me', closedAt: `2026-0${number}-01T00:00:00Z` }) + ); + const toast = shippedToast(shipped, 'me'); + // no single `pull` for a batch; the list carries every relevant pull, + // ranked newest-closed-first (all tie on tier here, since every one is + // 'yours') so the renderer's top-N slice and "+N more" fold both read + // off the same ranked array instead of a separately-tracked count + expect(toast?.pull).toBeUndefined(); + expect(toast?.pulls?.map(p => p.number)).toEqual([5, 4, 3, 2, 1]); + const shown = toast?.pulls?.slice(0, TOAST_PULLS_SHOWN) ?? []; + expect(shown.map(p => p.number)).toEqual([5, 4, 3]); + const overflow = (toast?.pulls?.length ?? 0) - TOAST_PULLS_SHOWN; + expect(overflow).toBe(2); + }); + it('re-fires on a newer merge but is stable for the same backlog', () => { const a = dp({ number: 1, author: 'me', closedAt: '2026-01-01T00:00:00Z' }); const b = dp({ number: 2, author: 'me', closedAt: '2026-01-02T00:00:00Z' }); diff --git a/frontend-v2/src/model/shipped.ts b/frontend-v2/src/model/shipped.ts index 70b4bdf1..576c60b5 100644 --- a/frontend-v2/src/model/shipped.ts +++ b/frontend-v2/src/model/shipped.ts @@ -75,8 +75,13 @@ export function shippedToast(shipped: PullData[], me: string): Toast | null { ? 'Your PR landed.' : 'One you reviewed landed.' : breakdown, - // a single relevant merge gets a link to it; a batch stays a summary + // a single relevant merge gets a link to it; a batch lists the ranked + // PRs instead (the renderer shows the first few and folds the rest + // into a "+N more" line, per TOAST_PULLS_SHOWN) so a multi-ship + // catch-up names real work instead of collapsing to a bare count pull: n === 1 ? { repo: top.repo, number: top.number, title: top.title } : undefined, + pulls: + n > 1 ? ranked.map(p => ({ repo: p.repo, number: p.number, title: p.title })) : undefined, dedupeKey: `shipped:${latest}:${n}`, }; } diff --git a/frontend-v2/src/model/status.test.ts b/frontend-v2/src/model/status.test.ts index 863ba754..71c99478 100644 --- a/frontend-v2/src/model/status.test.ts +++ b/frontend-v2/src/model/status.test.ts @@ -298,6 +298,17 @@ describe('changesRequestedBy / engagedNoStamp', () => { expect(d.changesRequestedBy).toEqual(['grumpy']); }); + it('changesRequestedBy excludes bots', () => { + const p = withStatus({ + unstamped_reviewers: [ + { login: 'grumpy', state: 'CHANGES_REQUESTED', date: 1 }, + { login: 'dependabot[bot]', state: 'CHANGES_REQUESTED', date: 2 }, + ], + }); + const d = derive(p, undefined, NOW); + expect(d.changesRequestedBy).toEqual(['grumpy']); + }); + it('engagedNoStamp includes unstamped reviewers of any verdict', () => { const p = withStatus({ unstamped_reviewers: [ diff --git a/frontend-v2/src/model/toast.ts b/frontend-v2/src/model/toast.ts index f0fd8c9b..0a0d9b40 100644 --- a/frontend-v2/src/model/toast.ts +++ b/frontend-v2/src/model/toast.ts @@ -1,5 +1,10 @@ export type ToastTone = 'reward' | 'nag' | 'info'; +/** How many of a batched toast's `pulls` render as links before the rest + * fold into a "+N more" line. Shared by the toast stack and the notification + * panel so a batched nudge lists the same number of PRs in both places. */ +export const TOAST_PULLS_SHOWN = 3; + /** * The reusable toast contract. Both the gamified cheers evaluator * (model/cheers) and the shipped catch-up (model/shipped) produce these; @@ -30,6 +35,13 @@ export interface Toast { * on GitHub if off-screen), and the card renders its number + title as a * link, so a toast says WHICH pull, not just a bare #number */ pull?: { repo: string; number: number; title?: string }; + /** for a batched toast covering more than one PR (e.g. the shipped + * catch-up when several landed at once): the pulls it covers, ranked + * most-relevant-first. The renderer shows the first `TOAST_PULLS_SHOWN` + * as links and folds the rest into a "+N more" line, the same overflow + * pattern the board's other lists use. Falls back to the singular `pull` + * above when absent or empty. */ + pulls?: { repo: string; number: number; title?: string }[]; /** stable id so the same logical toast isn't re-fired every board tick */ dedupeKey?: string; /** per-toast lifetime override (ms); falls back to the tone default */ diff --git a/frontend-v2/src/store.test.ts b/frontend-v2/src/store.test.ts new file mode 100644 index 00000000..20b6c1a3 --- /dev/null +++ b/frontend-v2/src/store.test.ts @@ -0,0 +1,147 @@ +import { describe, expect, it } from 'vitest'; +import { pullKey } from '../../shared/format'; +import type { PullData } from '../../shared/types'; +import { isSnoozed, type SnoozeRecord } from './store'; + +// isSnoozed reads only repo/number/updated_at and a few status counts, so a +// minimal pull is enough; the outer cast keeps the fixture to what it touches. +// cr/qa/unstamped take either a plain count (all "human" logins) or an +// explicit login list, so a test can mark specific entries as bots. +function pull(o: { + updatedAtEpoch: number; + comments?: number; + humanComments?: number; + cr?: number | string[]; + qa?: number | string[]; + unstamped?: number | string[]; +}): PullData { + const logins = (n: number | string[] | undefined, fallback: string) => + Array.isArray(n) ? n : Array.from({ length: n ?? 0 }, () => fallback); + return { + repo: 'org/repo', + number: 7, + updated_at: new Date(o.updatedAtEpoch * 1000).toISOString(), + status: { + cr_req: 2, + qa_req: 1, + allCR: logins(o.cr, 'human').map(login => ({ data: { user: { login } } })), + allQA: logins(o.qa, 'human').map(login => ({ data: { user: { login } } })), + dev_block: [], + deploy_block: [], + commit_statuses: [], + comment_count: o.comments ?? 0, + human_comment_count: o.humanComments, + unstamped_reviewers: logins(o.unstamped, 'human').map(login => ({ + login, + state: 'COMMENTED', + date: 0, + })), + }, + } as unknown as PullData; +} + +const AT = 1_000_000; // epoch secs the pull was snoozed +const WITHIN = AT + 60; // "now", inside the 24h window +const KEY = pullKey({ repo: 'org/repo', number: 7 }); +const rec = (o: Partial = {}): Record => ({ + [KEY]: { at: AT, comments: 0, cr: 0, qa: 0, unstamped: 0, ...o }, +}); + +describe('isSnoozed', () => { + it('stays snoozed when nothing changed since the snooze', () => { + expect(isSnoozed(pull({ updatedAtEpoch: AT - 100 }), rec(), WITHIN)).toBe(true); + }); + + it('is not snoozed without a record for the pull', () => { + expect(isSnoozed(pull({ updatedAtEpoch: AT - 100 }), {}, WITHIN)).toBe(false); + }); + + it('wakes when the comment count climbs above the baseline', () => { + expect( + isSnoozed(pull({ updatedAtEpoch: AT - 100, comments: 3 }), rec({ comments: 2 }), WITHIN) + ).toBe(false); + }); + + it('wakes on a new review: a CR stamp or an unstamped verdict', () => { + expect(isSnoozed(pull({ updatedAtEpoch: AT - 100, cr: 1 }), rec({ cr: 0 }), WITHIN)).toBe( + false + ); + expect( + isSnoozed(pull({ updatedAtEpoch: AT - 100, unstamped: 1 }), rec({ unstamped: 0 }), WITHIN) + ).toBe(false); + }); + + it('wakes when a reviewer moves from COMMENTED to APPROVED while snoozed', () => { + // at snooze time: one human COMMENTED verdict sitting in + // unstamped_reviewers, no CR stamp yet. By the time of this check they + // approved: cr climbs to 1 and unstamped drops to 0. A single combined + // "reviews" counter (cr+qa+unstamped) would read this as unchanged + // (1 -> 1) and stay asleep; cr and unstamped tracked separately must + // each be compared to their own baseline so the cr climb alone wakes it. + expect( + isSnoozed( + pull({ updatedAtEpoch: AT - 100, cr: 1, unstamped: 0 }), + rec({ cr: 0, unstamped: 1 }), + WITHIN + ) + ).toBe(false); + }); + + it('does not wake when a count merely drops (a deleted comment fails safe)', () => { + expect( + isSnoozed(pull({ updatedAtEpoch: AT - 100, comments: 1 }), rec({ comments: 3 }), WITHIN) + ).toBe(true); + }); + + it('wakes on a push that moved updated_at past the snooze', () => { + expect(isSnoozed(pull({ updatedAtEpoch: AT + 100 }), rec(), WITHIN)).toBe(false); + }); + + it('expires after the 24h window', () => { + expect(isSnoozed(pull({ updatedAtEpoch: AT - 100 }), rec(), AT + 24 * 3600 + 1)).toBe(false); + }); + + it('a bot review or comment-only verdict does not wake a snooze', () => { + // claude[bot] posts a CR stamp and a COMMENTED verdict on the same + // pull; bot activity must never drive the human-review nudge, so + // neither should wake a snoozed row. + expect( + isSnoozed(pull({ updatedAtEpoch: AT - 100, cr: ['claude[bot]'] }), rec({ cr: 0 }), WITHIN) + ).toBe(true); + expect( + isSnoozed( + pull({ updatedAtEpoch: AT - 100, unstamped: ['claude[bot]'] }), + rec({ unstamped: 0 }), + WITHIN + ) + ).toBe(true); + }); + + it('still wakes on a human review alongside a bot one', () => { + expect( + isSnoozed( + pull({ updatedAtEpoch: AT - 100, cr: ['claude[bot]', 'human'] }), + rec({ cr: 0 }), + WITHIN + ) + ).toBe(false); + }); + + it('prefers human_comment_count over comment_count for the wake signal', () => { + // The server counts a bot comment in comment_count but not in + // human_comment_count; only the human-only count should matter. + expect( + isSnoozed( + pull({ updatedAtEpoch: AT - 100, comments: 3, humanComments: 2 }), + rec({ comments: 2 }), + WITHIN + ) + ).toBe(true); + }); + + it('falls back to comment_count when an older server omits human_comment_count', () => { + expect( + isSnoozed(pull({ updatedAtEpoch: AT - 100, comments: 3 }), rec({ comments: 2 }), WITHIN) + ).toBe(false); + }); +}); diff --git a/frontend-v2/src/store.ts b/frontend-v2/src/store.ts index 6cc35be0..57831b53 100644 --- a/frontend-v2/src/store.ts +++ b/frontend-v2/src/store.ts @@ -6,6 +6,7 @@ import { type DerivedPull, type Weight, } from '../../shared/model/status'; +import { isBotLogin } from '../../shared/model/visibility'; import { getSettings, subscribeSettings } from './settings'; import { epoch, pullKey } from '../../shared/format'; import { readStorage, writeStorage } from './storage'; @@ -33,9 +34,10 @@ export interface Snapshot { lastPayloadAt: number; /** epoch secs of the last Clear — the "Recently updated" baseline */ lastSeen: number; - /** pull key → epoch secs it was snoozed: hidden for a day or until it - * changes. Persisted per-browser. */ - snoozed: Readonly>; + /** pull key → snooze record (when it was snoozed, plus the comment and + * review baseline that wakes it): hidden for a day or until it changes. + * Persisted per-browser. */ + snoozed: Readonly>; /** "Refresh all" in progress (or just finished): how many of the pulls * queued at kickoff have reported back. Null when no refresh is running. */ refreshProgress: { done: number; total: number } | null; @@ -109,24 +111,72 @@ export function isFresh(d: Pick, lastSeenAt: number) { return epoch(d.updated_at) > lastSeenAt; } -// Snooze: "not today" for one PR. A snooze lasts a day, and any change to -// the pull (a push, a comment — anything that moves updated_at) voids it -// early: punting a PR must never hide what happens to it next. +// Snooze: "not today" for one PR. A snooze lasts a day, and any activity on +// the pull voids it early: punting a PR must never hide what happens to it +// next. Two kinds of activity count. A push, edit, or label moves updated_at +// (the classic check). A new comment or review does NOT move updated_at +// (those arrive on their own webhook path), so the snooze also records the +// comment count plus CR/QA/unstamped counts at snooze time and wakes when +// any of them climbs. The three review counts are kept separate rather than +// summed: a reviewer moving from COMMENTED to APPROVED drops unstamped by +// one and raises cr by one, a wash under one combined total that would leave +// a snooze asleep through exactly the transition it should wake for. Counts +// only: the wire has no per-event author, so your own comment wakes it too, +// the same way your own push already does. +export type SnoozeRecord = { + at: number; + comments: number; + cr: number; + qa: number; + unstamped: number; +}; const SNOOZED_KEY = 'pd2.snoozed'; const SNOOZE_SECS = 24 * 3600; -let snoozed: Record = {}; +// the comment/review signals that wake a snooze, read off the pull's status. +// Bot activity (claude[bot]'s review, a CI bot's comment) must never wake a +// snooze on its own, so every count here excludes bot logins first: prefer +// the server's human-only comment count when it's sent, and re-derive cr/qa/ +// unstamped from the same isBotLogin the board uses everywhere else. +const snoozeActivity = (d: Pick) => { + const isHuman = (login: string) => !isBotLogin(login, extraBots); + return { + comments: d.status.human_comment_count ?? d.status.comment_count ?? 0, + cr: d.status.allCR.filter(s => isHuman(s.data.user.login)).length, + qa: d.status.allQA.filter(s => isHuman(s.data.user.login)).length, + unstamped: d.status.unstamped_reviewers?.filter(r => isHuman(r.login)).length ?? 0, + }; +}; +let snoozed: Record = {}; try { - snoozed = JSON.parse(readStorage(SNOOZED_KEY) ?? '{}') ?? {}; + const raw = JSON.parse(readStorage(SNOOZED_KEY) ?? '{}') ?? {}; + // clean cut: only the full {at, comments, cr, qa, unstamped} shape is + // supported. A half-shaped entry (the old {at, comments, reviews} baseline, + // or anything from before it) is dropped, so the pull reappears once and + // you re-snooze it if you still want. + for (const [k, v] of Object.entries(raw)) { + const r = v as Partial | null; + if ( + r && + typeof r === 'object' && + typeof r.at === 'number' && + typeof r.comments === 'number' && + typeof r.cr === 'number' && + typeof r.qa === 'number' && + typeof r.unstamped === 'number' + ) + snoozed[k] = r as SnoozeRecord; + } } catch { snoozed = {}; } const saveSnoozed = () => { const now = Date.now() / 1000; - for (const [k, at] of Object.entries(snoozed)) if (now > at + SNOOZE_SECS) delete snoozed[k]; + for (const [k, rec] of Object.entries(snoozed)) + if (now > rec.at + SNOOZE_SECS) delete snoozed[k]; writeStorage(SNOOZED_KEY, JSON.stringify(snoozed)); }; -export function snoozePull(key: string) { - snoozed[key] = Date.now() / 1000; +export function snoozePull(pull: Pick) { + snoozed[pullKey(pull)] = { at: Date.now() / 1000, ...snoozeActivity(pull) }; saveSnoozed(); schedulePublish(); } @@ -142,15 +192,22 @@ export function clearSnoozes() { schedulePublish(); } -/** Snoozed and nothing has happened since: still hidden. */ +/** Snoozed and nothing has happened since: still hidden. Wakes on a push + * (updated_at moved) or a new comment or review (a count above the baseline + * captured at snooze time). */ export function isSnoozed( - d: Pick, - snoozedAt: Readonly>, + d: Pick, + snoozedAt: Readonly>, now: number = Date.now() / 1000 ) { - const at = snoozedAt[pullKey(d)]; - if (at == null) return false; - return now < at + SNOOZE_SECS && epoch(d.updated_at) <= at; + const rec = snoozedAt[pullKey(d)]; + if (rec == null) return false; + if (now >= rec.at + SNOOZE_SECS) return false; + if (epoch(d.updated_at) > rec.at) return false; + const a = snoozeActivity(d); + if (a.comments > rec.comments || a.cr > rec.cr || a.qa > rec.qa || a.unstamped > rec.unstamped) + return false; + return true; } let snapshot: Snapshot = { diff --git a/frontend-v2/src/styles.css b/frontend-v2/src/styles.css index 1a1671c9..01bcafc4 100644 --- a/frontend-v2/src/styles.css +++ b/frontend-v2/src/styles.css @@ -726,6 +726,29 @@ label:has(> input:not(:disabled)) { animation: spin-once 400ms var(--ease-quart); } +/* the empty-search exclamation punching in after the glass draws — one shot, + scale + fade from its own center (fill-box), delayed so it lands last */ +@keyframes alert-pop { + from { + opacity: 0; + scale: 0.4; + } + to { + opacity: 1; + scale: 1; + } +} +.alert-pop { + transform-box: fill-box; + transform-origin: center; + animation: alert-pop 380ms var(--ease-quint) 300ms backwards; +} +@media (prefers-reduced-motion: reduce) { + .alert-pop { + animation: none; + } +} + @keyframes nod { 50% { scale: 1.25; diff --git a/frontend-v2/src/toasts.tsx b/frontend-v2/src/toasts.tsx index 3b604706..c62947bd 100644 --- a/frontend-v2/src/toasts.tsx +++ b/frontend-v2/src/toasts.tsx @@ -1,6 +1,6 @@ import { useCallback, useEffect, useMemo, useRef, useState } from 'react'; import { X } from 'lucide-react'; -import { githubUrl, rowDomId } from '../../shared/format'; +import { githubUrl, pullKey, rowDomId, shortRepo } from '../../shared/format'; import { Icon } from './components/Icon'; import { type CheerBaseline, @@ -12,7 +12,7 @@ import { type ToastKind, } from './model/cheers'; import type { DerivedPull } from '../../shared/model/status'; -import type { Toast } from './model/toast'; +import { type Toast, TOAST_PULLS_SHOWN } from './model/toast'; import { getSettings } from './settings'; import { readSessionStorage, writeSessionStorage } from './storage'; import type { PullData } from '../../shared/types'; @@ -554,21 +554,42 @@ function ToastCard({ toast, onDismiss }: { toast: LiveToast; onDismiss: (id: num )} {toast.title}
- {toast.pull && ( - e.stopPropagation()} - className="mt-0.5 block truncate text-xs font-medium text-brand hover:underline" - > - #{toast.pull.number} - {toast.pull.title ? ` ${toast.pull.title}` : ''} - - )} - {toast.body && ( - {toast.body} + {toast.pulls && toast.pulls.length > 0 ? ( + + {toast.pulls.slice(0, TOAST_PULLS_SHOWN).map(p => ( + e.stopPropagation()} + className="block text-xs font-medium text-brand hover:underline" + > + {shortRepo(p.repo)}#{p.number} + {p.title ? ` ${p.title}` : ''} + + ))} + {toast.pulls.length > TOAST_PULLS_SHOWN && ( + + +{toast.pulls.length - TOAST_PULLS_SHOWN} more + + )} + + ) : ( + toast.pull && ( + e.stopPropagation()} + className="mt-0.5 block text-xs font-medium text-brand hover:underline" + > + {shortRepo(toast.pull.repo)}#{toast.pull.number} + {toast.pull.title ? ` ${toast.pull.title}` : ''} + + ) )} + {toast.body && {toast.body}} {toast.actionLabel && toast.onAction && (