diff --git a/controllers/api.js b/controllers/api.js index 8a679df2..a06808de 100644 --- a/controllers/api.js +++ b/controllers/api.js @@ -34,6 +34,12 @@ function toRecord(d) { cr: { req: p.status.cr_req, have: d.crHave, by: d.crBy, recr_by: d.recrBy }, qa: { req: p.status.qa_req, have: d.qaHave, by: d.qaBy }, }, + review: { + own: d.ownReview, + author_is_developer: d.authorIsDeveloper, + asked_of: d.askedOf, + asked_at: d.askedAt, + }, dev_blocked_by: d.devBlockedBy, deploy_blocked_by: d.deployBlockedBy, head_sha: p.head.sha, diff --git a/controllers/githubHooks.js b/controllers/githubHooks.js index 80fc4f12..4e6f19de 100644 --- a/controllers/githubHooks.js +++ b/controllers/githubHooks.js @@ -13,6 +13,17 @@ import getLogin from '../lib/get-user-login.js'; import utils from '../lib/utils.js'; import dbManager from '../lib/db-manager.js'; import git from '../lib/git-manager.js'; +import { isBaseMerge } from '../lib/base-merge.js'; + +// how long a push waits on GitHub to say what its head commit is +const COMMIT_LOOKUP_MS = 3000; +function withTimeout(promise, ms) { + let timer; + const late = new Promise((_, reject) => { + timer = setTimeout(() => reject(new Error('timed out')), ms); + }); + return Promise.race([promise, late]).finally(() => clearTimeout(timer)); +} import { projectSettings } from '../lib/projects.js'; import { forgetIssue, workIssueTouched, workPullTouched } from '../lib/work.js'; @@ -92,11 +103,31 @@ const HooksController = { // Clear prior CR/QA signoffs immediately for snappy feedback. Emoji // CR/QA stay cleared; a carried-over GitHub *approval* is restored by // the full refresh below (see Signature.parseReview / git-manager). - preUpdate = dbManager.invalidateSignatures( - body.repository.full_name, - body.pull_request.number, - ['QA', 'CR'] - ); + // Skip it only for a push that is nothing but a merge of the base + // ("Update branch"): stamps survive those, and the refresh below + // settles the rest. A merge on top of other new commits (its first + // parent isn't the old head) carries real code, so it invalidates. + // The lookup is capped so a slow GitHub can't hold the reply; any + // error or timeout falls back to invalidating. + preUpdate = withTimeout( + Promise.resolve(git.getCommit(body.repository.full_name, body.after)), + COMMIT_LOOKUP_MS + ) + .then( + commit => + commit.parents[0]?.sha === body.before && + isBaseMerge(commit, body.pull_request.base.ref) + ) + .catch(() => false) + .then(isMerge => + isMerge + ? undefined + : dbManager.invalidateSignatures( + body.repository.full_name, + body.pull_request.number, + ['QA', 'CR'] + ) + ); reconcileAfterUpdate = true; } @@ -329,17 +360,25 @@ function handleLabelEvents(body) { * Record one `review_requested` / `review_request_removed` webhook's * metadata onto the in-memory review_requests cache (models/pull.js) -- * memory-only, no DB write. `body.requested_reviewer` is absent for a team - * review request, which Pulldasher doesn't track per-reviewer, so that's a - * no-op. + * review request (`requested_team` instead); its time goes to the team cache, + * and the slug rides on the pull payload's requested_teams, saved with the pull. */ function recordReviewRequestMetadata(body) { const reviewer = body.requested_reviewer; + const repo = body.repository.full_name; + const number = body.pull_request.number; + if (!reviewer) { + // a team: only the time is kept; the slug rides on the pull payload + const slug = body.requested_team?.slug; + if (slug && body.action === 'review_request_removed') { + Pull.recordTeamRequestRemoved(repo, number, slug); + } else if (slug) { + Pull.recordTeamRequested(repo, number, slug, Math.floor(Date.now() / 1000)); + } return Promise.resolve(); } - const repo = body.repository.full_name; - const number = body.pull_request.number; const login = getLogin(reviewer); if (body.action === 'review_request_removed') { diff --git a/controllers/settings.js b/controllers/settings.js index 3ab5ab58..bf4944f1 100644 --- a/controllers/settings.js +++ b/controllers/settings.js @@ -1,4 +1,5 @@ import { projectSettings } from '../lib/projects.js'; +import pullManager from '../lib/pull-manager.js'; import { saveSetting, settingsLoaded } from '../lib/settings.js'; import { checkDecideRotation, @@ -96,7 +97,10 @@ export default { if (has(body, 'developer_teams')) { const checked = checkDeveloperTeams(body.developer_teams); if (checked.error) return res.status(400).json({ error: checked.error }); - saves.push(() => saveSetting('developer_teams', checked.teams, req.roadmapLogin)); + saves.push(async () => { + await saveSetting('developer_teams', checked.teams, req.roadmapLogin); + pullManager.developerTeamsChanged(); + }); } if (has(body, 'ongoing_projects')) { const checked = checkOngoingProjects(body.ongoing_projects); diff --git a/controllers/stats.js b/controllers/stats.js index bedc03a6..779efa26 100644 --- a/controllers/stats.js +++ b/controllers/stats.js @@ -72,7 +72,9 @@ function queryMergeAgeByDay() { /** * Per-pull hours from open to first CR signature, for merged pulls in the - * last 12 months. One row per (repo, number) — used both to compute the + * last 12 months. Only someone other than the author counts: under + * self-review the author's own CR would read as zero, and a pull with only + * that has no first review and drops out. One row per (repo, number) — used both to compute the * monthly average in SQL and, in JS, the monthly median (MySQL has no * MEDIAN aggregate, and the row count here is small enough — thousands, not * millions — that pulling the raw latencies and sorting in JS is simpler @@ -86,10 +88,11 @@ function queryFirstCrLatencies() { (cr.min_date - p.date) AS latency_seconds \ FROM pulls p \ JOIN ( \ - SELECT repo, number, MIN(date) AS min_date \ - FROM pull_signatures \ - WHERE type = 'CR' \ - GROUP BY repo, number \ + SELECT s.repo, s.number, MIN(s.date) AS min_date \ + FROM pull_signatures s \ + JOIN pulls o ON o.repo = s.repo AND o.number = s.number \ + WHERE s.type = 'CR' AND s.user <> o.owner \ + GROUP BY s.repo, s.number \ ) cr ON cr.repo = p.repo AND cr.number = p.number \ WHERE p.date_merged >= ? \ AND cr.min_date > p.date"; @@ -209,7 +212,7 @@ function loadHistory() { // factored out so they're independently testable without a live DB connection // (mirrors lib/refresh.js exporting processIssueItem/processPullItem for the // same reason). -export { firstCrByMonth, median, formatDate, formatIsoWeek, round1 }; +export { queryFirstCrLatencies, firstCrByMonth, median, formatDate, formatIsoWeek, round1 }; export default { /** diff --git a/frontend-v2/src/app.tsx b/frontend-v2/src/app.tsx index 0fff1684..37e4fd0c 100644 --- a/frontend-v2/src/app.tsx +++ b/frontend-v2/src/app.tsx @@ -11,16 +11,15 @@ import { } from 'react'; import { ago, closedEpoch, n, pullKey, shortRepo } from '../../shared/format'; import type { ActionStateKey } from './model/actions'; -import { actionState } from './model/actions'; +import { actionState, askedOf } from './model/actions'; import { DEFAULT_RANGE, DEFAULT_SORT, LENS_LABELS, ORIGIN_KEYS, ZOOM_KEY, type Lens } from './lens'; import type { DerivedPull } from '../../shared/model/status'; 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, refreshAllText, usePulldasher } from './store'; +import { isSnoozed, refreshAllText, usePulldasher } from './store'; import { primeScope, useScope } from './prefs'; import { useBoardHotkeys, useHeaderHeightVar } from './hooks'; import { ageRotDays, getSettings, type Settings as SettingsShape, useSettings } from './settings'; @@ -28,7 +27,6 @@ import { useNotifications } from './notifications'; import { ToastStack, useToasts } from './toasts'; import { requestNames, useNames } from './model/names'; import { CRYO_KEY, isBotLogin, personHidden, repoHidden } from '../../shared/model/visibility'; -import { reviewRequestedFrom } from './model/reviewers'; import { CornerBadge, QuietButton, textInputClass } from './components/bits'; import { foldDomId, openFold } from './components/Lane'; import { Legend } from './components/Legend'; @@ -300,12 +298,6 @@ export function App() { snoozed, projectLabelPrefix, } = usePulldasher(); - // per-repo reviewer pools and whose turn each starved, unclaimed pull is — - // computed ONCE over the whole board and shared by the rows (rowOpts), - // desktop notifications, and the cheers evaluator, so the three surfaces - // can never disagree about the rotation (and never re-derive it in - // parallel; buildReviewerPools walks every pull's signatures) - const pools = useMemo(() => buildReviewerPools(pulls), [pulls]); // Publish the widest CR/QA required counts on the board so every row's pip // slot pads to them (styles.css .pd-pip-slot-*) and the marks line up in a // column. Over ALL open pulls, not the filtered view: simpler, and the @@ -324,14 +316,6 @@ export function App() { root.setProperty('--cr-slots', String(maxCrReq)); root.setProperty('--qa-slots', String(maxQaReq)); }, [maxCrReq, maxQaReq]); - const turns = useMemo(() => { - const m = new Map(); - for (const p of pulls) { - const who = turnFor(p, pools, pulls); - if (who) m.set(pullKey(p.data), who); - } - return m; - }, [pulls, pools]); // resolve every author on the board to a human display name (batched, // cached — model/names.ts): the person hover-cards, the people pickers, // and name-aware search all read from this one map @@ -625,7 +609,7 @@ export function App() { p.data.draft && p.data.user.login !== me && !revealedAuthor(p.data.user.login) && - !reviewRequestedFrom(p, me); + !askedOf(p).includes(me); return hidden || cryoHidden || draftHidden; }, [ @@ -700,7 +684,7 @@ export function App() { ); // desktop notifications watch the whole board (minus the standing hidden // settings above), not the current filter - useNotifications(nudgeablePulls, me, turns, initialized); + useNotifications(nudgeablePulls, me, initialized); // scope + hidden, but deliberately NOT free text: a query drives the global // Search lens (App renders instead of the current lens), so @@ -840,7 +824,7 @@ export function App() { const c = { parked: 0, drafts: 0, hiddenRepos: 0, hiddenPeople: 0, bots: 0, hiddenNow: 0 }; for (const p of pulls) { if (p.cryo) c.parked++; - if (p.data.draft && p.data.user.login !== me && !reviewRequestedFrom(p, me)) c.drafts++; + if (p.data.draft && p.data.user.login !== me && !askedOf(p).includes(me)) c.drafts++; if (repoHidden(p.data.repo, settings.repoPrefs)) c.hiddenRepos++; if ( p.data.user.login !== me && @@ -930,8 +914,6 @@ export function App() { compact: settings.density === 'compact', laneCap: settings.laneCapByLens[lens] ?? settings.laneCap, parentOf, - pools, - turns, isBotAuthor, onProject, }), @@ -947,8 +929,6 @@ export function App() { settings.laneCapByLens, lens, parentOf, - pools, - turns, isBotAuthor, onProject, ] @@ -1047,15 +1027,6 @@ export function App() { setLens('review'); setWeightSel(['XS', 'S']); }, []); - // the "your turn" toast's Claim button: claim the review straight from the - // nudge (which also adds you as a GitHub reviewer, same as any claim) - const onClaimTurn = useCallback( - (repo: string, number: number) => { - const p = pulls.find(x => x.data.repo === repo && x.data.number === number); - if (p) claimReview(p.data); - }, - [pulls] - ); // pull keys the viewer snoozed in Review, so cheers skips them the way the // Review lane does — a snooze is "not today", not "nudge me anyway" const snoozedKeys = useMemo( @@ -1072,11 +1043,9 @@ export function App() { } = useToasts( nudgeablePulls, me, - turns, shippedExtras, closed, onQuickWins, - onClaimTurn, initialized, names, snoozedKeys, @@ -1084,7 +1053,7 @@ export function App() { ); // A fired nudge outlives the PR it points at: once that PR merges or closes - // it drops off the board, so a bell entry like "return the favor on + // it drops off the board, so a bell entry like "review requested on // fixbot#3116" is left pointing at a dead link. Drop any panel record whose // PR has left the open board -- notificationStale is general across every // kind, and exempts the retrospective shipped recap and board-wide rewards. @@ -1328,11 +1297,11 @@ export function App() { sessionActive={sessionActive} inputProps={{ 'aria-label': - 'Search all PRs, open and closed: text, #number, label:x, status:x, older:5, repo:x, author:x, weight:xs, has:action, is:draft, is:mine, is:restamp, is:blocked, is:bot', + 'Search all PRs, open and closed: text, #number, label:x, status:x, older:5, repo:x, author:x, weight:xs, has:action, is:draft, is:mine, is:restamp, is:asked, is:blocked, is:bot', // "/" finds within a view that has its own find box // (hooks.ts), so the hint only shows where "/" comes here placeholder: viewFinds ? 'Search all PRs' : 'Search all PRs (press /)', - title: 'text, #number, label:x, status:x, older:5, repo:x, author:x, weight:xs, has:action, is:draft, is:mine, is:restamp, is:blocked, is:bot', + title: 'text, #number, label:x, status:x, older:5, repo:x, author:x, weight:xs, has:action, is:draft, is:mine, is:restamp, is:asked, is:blocked, is:bot', className: `w-[210px] max-w-full grow pr-2.5 pl-8 sm:grow-0 ${textInputClass}`, }} /> @@ -1446,7 +1415,7 @@ export function App() { )} {initialized && !searching && lens === 'stats' && ( - + )} diff --git a/frontend-v2/src/backend/dummy.ts b/frontend-v2/src/backend/dummy.ts index d7e91e02..4a53b539 100644 --- a/frontend-v2/src/backend/dummy.ts +++ b/frontend-v2/src/backend/dummy.ts @@ -44,7 +44,7 @@ export async function loadDummy(): Promise { const withBody = (p: PullData, i: number): PullData => p.body === 'pull request dummy body' ? { ...p, body: BODIES[i % BODIES.length] } : p; const pulls = withSyntheticStacks(raw).map((p, i) => - withProject(withBody(withSizes(redate(p, i), i), i), i) + withProject(withBody(withSizes(withSelfReviewDemos(redate(p, i), i), i), i), i) ); // ?projects=100 adds that many projects in flight, to design at real size const scaled = new URLSearchParams(location.search).has('projects') @@ -65,6 +65,7 @@ export async function loadDummy(): Promise { 'size: XL': 'XL', }, projectLabelPrefix: DUMMY_PROJECT_PREFIX, + developerTeams: dummyDeveloperTeams(pulls), // The fixture carries almost no closed/merged pulls and no diff sizes, so // the Stats lens (merge-time-by-size, leaderboards over shipped work) has // nothing to show. Synthesize a fortnight of merged PRs across the size @@ -73,6 +74,20 @@ export async function loadDummy(): Promise { }; } +// The roster of developers (config projects.developerTeams): every dummy author +// but two, who stay outside contributors whose pulls still need someone else's +// review. The viewer is always on it. +const OUTSIDE_CONTRIBUTORS = new Set(['notme', 'Michelle5102']); +function dummyDeveloperTeams(pulls: PullData[]): Record { + const logins = [...new Set([dummyUser(), ...pulls.map(p => p.user.login)])].filter( + l => !OUTSIDE_CONTRIBUTORS.has(l) && !l.endsWith('[bot]') + ); + const names = ['Store Front', 'Platform', 'Community']; + const teams: Record = Object.fromEntries(names.map(n => [n, []])); + logins.forEach((l, i) => teams[names[i % names.length]].push(l)); + return teams; +} + export const DUMMY_PROJECT_PREFIX = 'project:'; // The Projects tab reads project labels off the pulls, and the fixture @@ -244,8 +259,8 @@ function participantsFor(pull: PullData, i: number): string[] { i === CHANGES_REQUESTED_INDEX ? ['grumpy-reviewer'] : i === COMMENTED_INDEX - ? ['curious-commenter'] - : ['silent-lurker']; + ? ['curious-commenter'] + : ['silent-lurker']; return [...new Set([...(pull.participants ?? []), ...stampers, ...extra])]; } @@ -269,6 +284,87 @@ const wantsReviewRequest = (pull: PullData, i: number): boolean => pull.user.login !== dummyUser() && i % 3 === 1; const ASSIGNEE_INDEXES = new Set([6, 12]); +// The self-review policy demos, keyed by index like the rest: team requests +// (one for a team nobody is on the roster of), requests of the viewer with a +// clock in hours (one past four), developers' own stamped pulls, an author +// whose own stamp went stale, and input hints from the diff's paths. +// [team slug, hours since it was requested]; the one at 5 is past the four +// hours reviewers answer in +const TEAM_REQUESTS: Record> = { + 2: [['store-front', 3]], + 9: [['store-front', 5]], + 17: [['platform', 2]], + 20: [['community', 4.5]], + 30: [['design', 1]], +}; +const OWN_STAMPED = new Set([3, 8, 21]); +const OWN_STALE_STAMP = 24; +const INPUT_HINTS: Record = { + 4: ['migrations'], + 18: ['ci', 'agent-docs'], + 26: ['deploy', 'dependencies'], + 33: ['alerting'], + // the viewer's own open, unrequested pulls (the chain), so the author-side + // "worth asking for review?" hint shows on the board + 10: ['migrations'], + 12: ['ci', 'dependencies'], +}; + +function withSelfReviewDemos(pull: PullData, i: number): PullData { + const now = Date.now(); + const author = pull.user; + const stamp = (type: 'CR' | 'QA', active: boolean, hoursAgo: number) => ({ + data: { + repo: pull.repo, + number: pull.number, + user: { id: 1, ...author }, + type, + created_at: new Date(now - hoursAgo * 3_600_000).toISOString(), + active: active ? 1 : 0, + comment_id: 900000000 + i * 10 + (type === 'CR' ? 1 : 2), + }, + }); + const meAsked = (pull.requested_reviewers ?? []).includes(dummyUser()); + let out: PullData = { + ...pull, + requested_teams: (TEAM_REQUESTS[i] ?? []).map(([slug]) => slug), + team_requests: (TEAM_REQUESTS[i] ?? []).map(([slug, hoursAgo]) => ({ + slug, + at: Math.floor(now / 1000 - hoursAgo * 3600), + })), + input_hints: INPUT_HINTS[i] ?? [], + // asked hours ago; the one at 7 is past the four hours reviewers answer in + review_requests: meAsked + ? [ + { + login: dummyUser(), + at: Math.floor(now / 1000) - (i === 7 ? 5 : 1 + (i % 3)) * 3600, + self: false, + }, + ] + : pull.review_requests, + }; + if (INPUT_HINTS[i] && (CHAIN_INDEXES as readonly number[]).includes(i)) { + // unrequested, so the hint is the only nudge to ask anyone + out = { ...out, requested_reviewers: [], requested_teams: [], team_requests: [] }; + } + if (OWN_STAMPED.has(i) || i === OWN_STALE_STAMP) { + out = { + ...out, + requested_reviewers: [], + requested_teams: [], + team_requests: [], + review_requests: [], + status: { + ...out.status, + allCR: [stamp('CR', i !== OWN_STALE_STAMP, 6)], + allQA: i === OWN_STALE_STAMP ? [] : [stamp('QA', true, 5)], + }, + }; + } + return out; +} + function withSyntheticStacks(pulls: PullData[]): PullData[] { const out = [...pulls]; const rebase = (i: number, baseRef: string) => { @@ -329,8 +425,8 @@ function redate(pull: PullData, i: number): PullData { deletions, requested_reviewers: wantsReviewRequest(pull, i) ? [dummyUser()] - : (pull.requested_reviewers ?? []), - assignees: ASSIGNEE_INDEXES.has(i) ? [dummyUser()] : (pull.assignees ?? []), + : pull.requested_reviewers ?? [], + assignees: ASSIGNEE_INDEXES.has(i) ? [dummyUser()] : pull.assignees ?? [], }; } diff --git a/frontend-v2/src/backend/socket.ts b/frontend-v2/src/backend/socket.ts index 4fff914e..c10fbda5 100644 --- a/frontend-v2/src/backend/socket.ts +++ b/frontend-v2/src/backend/socket.ts @@ -45,6 +45,9 @@ export interface Backend { * server (a roadmap write, an issue added, a setting, a sync), so open * boards fetch it again; returns the unsubscribe. */ onProjectsChanged: (handler: () => void) => () => void; + /** Calls `handler` with the new developer roster when someone saves it, + * so open boards re-derive who owes a review; returns the unsubscribe. */ + onDeveloperTeams: (handler: (teams: Record | undefined) => void) => () => void; } function liveBackend(): Backend { @@ -188,6 +191,15 @@ function liveBackend(): Backend { s.off('initialize', reconnected); }; }, + onDeveloperTeams(handler) { + const s = getSocket(); + const heard = (data: { developerTeams?: Record }) => + handler(data.developerTeams); + s.on('developerTeamsChanged', heard); + return () => { + s.off('developerTeamsChanged', heard); + }; + }, onConnection(handler) { onState = handler; const s = getSocket(); @@ -258,6 +270,8 @@ function dummyBackend(): Backend { window.addEventListener('pd:projectsChanged', handler); return () => window.removeEventListener('pd:projectsChanged', handler); }, + // the dummy roster never changes + onDeveloperTeams: () => () => undefined, whoami: () => Promise.resolve(dummyUser()), onPulls(handler) { emit = handler; diff --git a/frontend-v2/src/components/JailMode.test.tsx b/frontend-v2/src/components/JailMode.test.tsx index 8e0395fe..94bac16d 100644 --- a/frontend-v2/src/components/JailMode.test.tsx +++ b/frontend-v2/src/components/JailMode.test.tsx @@ -12,8 +12,12 @@ import { JailMode } from './JailMode'; let n = 0; /** One of my open PRs, run through the real derive; `ready` waives CR and - * QA so it can merge now, otherwise it waits on CR. */ -const pull = (ageDays = 1, { ready = false } = {}): DerivedPull => { + * QA so it can merge now, otherwise it needs CR: my own to stamp (self-review), + * or `asked`'s, whom I requested a review from an hour ago. */ +const pull = ( + ageDays = 1, + { ready = false, asked = [] as string[], hints = [] as string[] } = {} +): DerivedPull => { n += 1; const at = new Date(Date.now() - ageDays * 86400_000 - 60_000).toISOString(); const data: PullData = { @@ -47,6 +51,9 @@ const pull = (ageDays = 1, { ready = false } = {}): DerivedPull => { }, labels: [], participants: [], + requested_reviewers: asked, + review_requests: asked.map(login => ({ login, at: Date.now() / 1000 - 3600, self: false })), + input_hints: hints, }; return derive(data, undefined); }; @@ -101,7 +108,8 @@ describe('JailMode while the cell is down', () => { it('lifts ready PRs to the top and counts them in the why line', () => { const ready = pull(2, { ready: true }); render([...many(7), ready]); - expect(headers()).toEqual(['Ready to merge', 'Waiting on others']); + // the rest are mine to stamp under self-review + expect(headers()).toEqual(['Ready to merge', 'Your move']); expect(document.querySelector('[role="dialog"] li a')?.textContent).toContain( ready.data.title ); @@ -164,23 +172,41 @@ describe('JailMode while the cell is down', () => { expect(listed()).toBe(8); }); - it('copies a nudge listing the PRs waiting on others', async () => { + it('copies a nudge listing only the PRs waiting on people I asked, naming them', async () => { const writeText = vi.fn(() => Promise.resolve()); Object.assign(navigator, { clipboard: { writeText } }); - const first = pull(3); - render([first, ...many(7)]); + const first = pull(3, { asked: ['bob'] }); + const second = pull(2, { asked: ['cy', 'dee'] }); + render([first, second, ...many(6)]); + expect(headers()).toEqual(['Your move', 'Waiting on others']); const copy = [...document.querySelectorAll('[role="dialog"] button')].find( b => b.textContent === 'Copy a nudge' ); await act(async () => copy?.click()); const text = (writeText.mock.calls[0] as unknown as [string])[0]; - expect(text.split('\n')).toHaveLength(9); + expect(text.split('\n')).toHaveLength(3); expect(text).toContain( - `• ${first.data.title} https://github.com/iFixit/ifixit/pull/${first.data.number}` + `• ${first.data.title} https://github.com/iFixit/ifixit/pull/${first.data.number} (bob)` ); + expect(text).toContain(`(cy, dee)`); expect(copy?.textContent).toBe('Copied'); }); + it('offers no nudge when I asked nobody: a self-review waits on no one', () => { + render(many(8)); + const copy = [...document.querySelectorAll('[role="dialog"] button')].find( + b => b.textContent === 'Copy a nudge' + ); + expect(copy).toBeUndefined(); + }); + + it('hints when a self-reviewed PR touches what usually deserves team input', () => { + render([pull(1, { hints: ['ci', 'migrations'] }), ...many(7)]); + expect(dialog()?.textContent).toContain( + 'touches CI and migrations · worth asking for review?' + ); + }); + it('lets a screen reader or voice control out with a lone click', () => { render(many(8)); const hold = [...document.querySelectorAll('[role="dialog"] button')].find( diff --git a/frontend-v2/src/components/JailMode.tsx b/frontend-v2/src/components/JailMode.tsx index c690e029..45926b14 100644 --- a/frontend-v2/src/components/JailMode.tsx +++ b/frontend-v2/src/components/JailMode.tsx @@ -3,7 +3,7 @@ import { createPortal } from 'react-dom'; import { Lock } from 'lucide-react'; import type { DerivedPull } from '../../../shared/model/status'; import { githubUrl } from '../../../shared/format'; -import { rowNote, rowWord } from '../model/actions'; +import { askedOf, askForInputHint, rowNote, rowWord } from '../model/actions'; import { STAGE_WORDS } from '../model/stage'; import { jailCase, @@ -582,22 +582,21 @@ function useLeaving(pulls: DerivedPull[], openIds: ReadonlySet): Derived return leaving.filter(p => !pulls.some(q => pullId(q) === pullId(p))); } -/** Slack-ready text asking for the reviews your waiting PRs need: each - * title, its link, and who it waits on. */ -function nudgeText(rows: { pull: DerivedPull }[], me: string): string { +/** Slack-ready text chasing the reviews you asked for: each title, its + * link, and who was asked. Only PRs with a request: a self-reviewed PR waits + * on nobody, so there's no one to nudge about it. */ +function nudgeText(rows: { pull: DerivedPull }[]): string { return [ - 'A few of my PRs are waiting on a review or a stamp:', - ...rows.map(({ pull: p }) => { - const who = rowNote(p, me).context; - return `• ${p.data.title} ${githubUrl(p.data.repo, p.data.number)}${ - who ? ` (${who})` : '' - }`; - }), + 'A few of my PRs are waiting on a review I asked for:', + ...rows.map( + ({ pull: p }) => + `• ${p.data.title} ${githubUrl(p.data.repo, p.data.number)} (${askedOf(p).join(', ')})` + ), ].join('\n'); } /** Copies nudgeText, and says whether it worked. */ -function CopyNudge({ rows, me }: { rows: { pull: DerivedPull }[]; me: string }) { +function CopyNudge({ rows }: { rows: { pull: DerivedPull }[] }) { const [copied, setCopied] = useState<'copied' | 'failed' | null>(null); const done = (result: 'copied' | 'failed') => { setCopied(result); @@ -607,7 +606,7 @@ function CopyNudge({ rows, me }: { rows: { pull: DerivedPull }[]; me: string }) { if (!navigator.clipboard) return done('failed'); - navigator.clipboard.writeText(nudgeText(rows, me)).then( + navigator.clipboard.writeText(nudgeText(rows)).then( () => done('copied'), () => done('failed') ); @@ -641,16 +640,21 @@ function JailList({ ? jailGroups(all, me) : { ready: [], move: [], waiting: all.map(pull => ({ pull })) }; const live = (rows: { pull: DerivedPull }[]) => rows.filter(r => !gone.has(pullId(r.pull))); + // the PRs waiting on people you asked, wherever they sit (a request gone + // quiet past 4 hours is Your move); the copy button rides on the first + // section holding one + const asked = (rows: { pull: DerivedPull }[]) => live(rows).filter(r => askedOf(r.pull).length); + const nudgeRows = asked([...groups.move, ...groups.waiting]); const sections = [ { title: STAGE_WORDS.ready, sub: 'Merge these first', rows: groups.ready, ready: true }, { title: 'Your move', sub: 'Waiting on you', rows: groups.move }, { title: 'Waiting on others', - sub: 'Nudge them, or close what you don’t need', + sub: 'Nudge whoever you asked, or close what you don’t need', rows: groups.waiting, - nudge: true, }, ].filter(g => g.rows.length); + const nudgeOn = sections.find(g => !g.ready && asked(g.rows).length)?.title; return (
{sections.map(g => ( @@ -661,11 +665,7 @@ function JailList({ title={g.title} sub={g.sub} count={live(g.rows).length} - headerExtra={ - g.nudge && live(g.rows).length ? ( - - ) : undefined - } + headerExtra={g.title === nudgeOn ? : undefined} /> )}
    @@ -712,6 +712,7 @@ function JailRow({ // stands in when there's no detail const move = word?.kind === 'do' ? word.word : null; const detail = context ?? (word?.kind === 'wait' ? word.word : null); + const hint = mine && !ready ? askForInputHint(p) : null; const old = p.ageDays > maxDays; return (
  • {detail}} + {hint && {hint}}
    • ); } diff --git a/frontend-v2/src/components/Legend.tsx b/frontend-v2/src/components/Legend.tsx index ef319df2..2a3b0695 100644 --- a/frontend-v2/src/components/Legend.tsx +++ b/frontend-v2/src/components/Legend.tsx @@ -147,16 +147,16 @@ export function Legend() { Claim} - def="hover a row for its verbs. Claiming adds you as a reviewer on the PR, so GitHub and the board both show you’re on it; the row moves to Waiting on you until you submit, release, or are removed" + term={self-review} + def="a developer reviews and tests their own PR unless they ask for a review. Nobody else owes it anything" /> review requested} - def="someone requested your review on GitHub. Lands in Waiting on you, and the board quiets its own suggestions for that PR" + def="the author asked people (or a team) for a review on GitHub. It leads their Waiting on you; answer within hours" /> your turn} - def="a review that sat too long, pointed at the best-matched reviewer. A nudge, not a lock: anyone can take it" + term={I’ll review} + def="hover a row for its verbs. It adds you as a reviewer on the PR, so GitHub and the board both show you’re on it; the row moves to Waiting on you until you submit, drop it, or are removed" /> @@ -173,6 +173,10 @@ export function Legend() { term={is:restamp} def="a push undid your stamp and the PR is reviewable again" /> + is:asked} + def="someone asked you for a review" + /> is:blocked} def="under a dev or deploy block, whoever holds it" diff --git a/frontend-v2/src/components/RegionHint.tsx b/frontend-v2/src/components/RegionHint.tsx index 0a605fc4..40f3c2a2 100644 --- a/frontend-v2/src/components/RegionHint.tsx +++ b/frontend-v2/src/components/RegionHint.tsx @@ -30,8 +30,8 @@ export function RegionHint() { Review a particular area? Add code regions in Settings. - A region is plain text matched against a PR’s title, labels, branch, and repo; matches - gather in their own section above the queue. + A region is plain text matched against a PR’s title, labels, branch, and repo; other + people’s self-reviews that match gather under Could use your input. )} {showSnooze && ( @@ -660,15 +670,12 @@ function RowImpl({ const d = pull.data; const key = pullKey(d); const fresh = freshKind(pull, opts); - // who's claimed to review this pull, and whose turn the rotation names — - // both optional, so a lens that hasn't threaded them (yet) just sees null - // and rowNote falls back to its base (pre-coordination) note + // who said they'd review this pull const claim = claimFor(d); - const turn = opts.turns?.get(key) ?? null; - const poolSize = opts.pools?.get(d.repo)?.length ?? 0; // the viewer-relative note (model/actions.ts): never both-null for an open // pull, so the one badge below always has something to say - const note = rowNote(pull, opts.me, { claim, turn }); + const note = rowNote(pull, opts.me, { claim }); + const askHint = opts.askHint && d.user.login === opts.me ? askForInputHint(pull) : null; // the wait badge may itself carry a "last commit …" — don't say it twice const showIterating = isIterating(pull) && !(note.context ?? '').includes('last commit'); // the smallest clean marker for a teammate: a tiny heart over their @@ -680,7 +687,7 @@ function RowImpl({ const regions = matchedRegions(pull, settings.codeRegions); // only worth asking the whole-board lookup when this row is stacked but // rendering flat (its parent isn't visible right above it already) - const orphanParent = pull.dependent && depth === 0 ? (opts.parentOf?.(pull) ?? null) : null; + const orphanParent = pull.dependent && depth === 0 ? opts.parentOf?.(pull) ?? null : null; return ( 0 && d.user.login !== opts.me && ( - {regions.length > 0 && !opts.hideRegionMark && ( + {regions.length > 0 && ( // the brand diamond leads and the matched region is NAMED in // quiet ink beside it — which of your regions hit must read // without a hover (a bare glyph hid it; owner call). No @@ -756,7 +765,9 @@ function RowImpl({ - )}