Follow the self-review policy

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,

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') {

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);

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 {
    /**

frontend-v2/src/app.tsx

@@ -11,24 +11,22 @@ 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';
 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<string, string>();
-      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 <Search> 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,19 +1043,17 @@ export function App() {
    } = useToasts(
       nudgeablePulls,
       me,
-      turns,
       shippedExtras,
       closed,
       onQuickWins,
-      onClaimTurn,
       initialized,
       names,
       snoozedKeys,
       reviewStanding.parked
    );
 
    // 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() {
                </Suspense>
             )}
             {initialized && !searching && lens === 'stats' && (
-               <Stats pulls={humans} closed={closed} me={me} onPerson={onPerson} />
+               <Stats pulls={humans} closed={closed} bots={extraBots} me={me} onPerson={onPerson} />
             )}
          </main>
       </>