diff --git a/bin/repair-stale-open-pulls b/bin/repair-stale-open-pulls index 6d64d253..c64e97e9 100755 --- a/bin/repair-stale-open-pulls +++ b/bin/repair-stale-open-pulls @@ -3,7 +3,7 @@ * Pulls whose `closed` webhook never landed stay open in the DB. Until the * stale-open check listed repos outside config.repos, nothing repaired them * there: 21 PRs closed 2026-09-26 to 09-30, while prod wrote into dev's DB, - * stayed open on prod (iFixit/ops#1055 in PR jail). The hourly repair now + * stayed open on prod (iFixit/ops#1055, counted against their owners' open-PR limits). The hourly repair now * catches these; this fixes them without waiting for a deploy. * * It lists the open pulls of every repo the DB holds open pulls in and, with diff --git a/frontend-v2/src/app.tsx b/frontend-v2/src/app.tsx index 37e4fd0c..4869b95f 100644 --- a/frontend-v2/src/app.tsx +++ b/frontend-v2/src/app.tsx @@ -57,7 +57,7 @@ import { Stats } from './views/Stats'; import type { ProjectsNav } from './views/Projects'; import { Search } from './views/Search'; import { Settings } from './components/Settings'; -import { JailMode } from './components/JailMode'; +import { FinishLine } from './components/FinishLine'; export type { Lens }; @@ -1142,7 +1142,12 @@ export function App() { > v1 board - + { + n += 1; + const at = new Date(Date.now() - ageDays * 86400_000 - 60_000).toISOString(); + const data: PullData = { + repo: 'iFixit/ifixit', + number: n, + state: 'open', + title: `PR ${n}`, + body: '', + draft: false, + created_at: at, + updated_at: at, + closed_at: null, + merged_at: null, + mergeable: true, + difficulty: null, + additions: 10, + deletions: 5, + changed_files: 2, + milestone: { title: null, due_on: null }, + head: { ref: 'branch', sha: 'abc', repo: { owner: { login: 'iFixit' } } }, + base: { ref: 'master' }, + user: { login: 'me' }, + status: { + cr_req: ready ? 0 : 1, + qa_req: ready ? 0 : 1, + allCR: [], + allQA: [], + dev_block: [], + deploy_block: [], + commit_statuses: [], + }, + 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); +}; +const many = (count: number) => Array.from({ length: count }, () => pull()); + +let root: Root | null = null; +const host = document.createElement('div'); +document.body.append(host); +// most of these watch it pop up, and close it the old way, by holding +beforeEach(() => setSettings({ finishLineMode: 'auto', finishLineHoldToClose: true })); +afterEach(() => { + act(() => root?.unmount()); + root = null; + localStorage.clear(); + vi.useRealTimers(); +}); + +function render(pulls: DerivedPull[]) { + const r = (root ??= createRoot(host)); + act(() => r.render()); +} + +/** the stats row's buttons, by their words */ +const stats = () => + [...document.querySelectorAll('#finish-stats button')].map( + b => b.textContent + ); +const stat = (text: string) => + [...document.querySelectorAll('#finish-stats button')].find( + b => b.textContent === text + ); +const listed = () => document.querySelectorAll('[role="dialog"] section li').length; +const dialog = () => document.querySelector('[role="dialog"]'); +const countdown = () => document.getElementById('finish-title')?.textContent; +const badge = () => + document.querySelector('[aria-label^="Finish"][aria-label$="open PRs"]'); +const esc = () => + act(() => { + document.body.dispatchEvent( + new KeyboardEvent('keydown', { + key: 'Escape', + bubbles: true, + cancelable: true, + }) + ); + }); +const headers = () => [...document.querySelectorAll('[role="dialog"] h3')].map(h => h.textContent); + +describe('FinishLine while it is up', () => { + it('follows new PRs and merges live', () => { + const first = many(8); + render(first); + expect(stats()).toEqual(['8 open PRs, aim for 7']); + expect(listed()).toBe(8); + + // a new PR arrives over the socket + render([...first, pull()]); + expect(stats()).toEqual(['9 open PRs, aim for 7']); + expect(listed()).toBe(9); + + expect(countdown()).toBe('Finish 2 PRs'); + + // merges bring you under the limits: it says so, then lets you go + vi.useFakeTimers(); + render(first.slice(0, 3)); + expect(stats()).toEqual([]); + expect(countdown()).toBe('All finished'); + expect(dialog()?.textContent).toContain('None of your open PRs are past your limits.'); + expect(listed()).toBe(0); + act(() => vi.advanceTimersByTime(2500)); + expect(dialog()).toBeNull(); + }); + + it('lifts ready PRs to the top and counts them in the stats', () => { + const ready = pull(2, { ready: true }); + render([...many(7), ready]); + // 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 + ); + expect(countdown()).toBe('Finish 1 PR'); + expect(stats()).toEqual(['8 open PRs, aim for 7', '1 PR ready to merge']); + expect(dialog()?.textContent).toContain('Only you see this.'); + }); + + it('only shows the badge until you turn the popup on', () => { + setSettings({ finishLineMode: DEFAULT_SETTINGS.finishLineMode }); + render(many(9)); + expect(dialog()).toBeNull(); + expect(badge()).not.toBeNull(); + }); + + it('only shows the badge when the popup is off', () => { + setSettings({ finishLineMode: 'manual' }); + render(many(9)); + expect(document.querySelector('[role="dialog"]')).toBeNull(); + expect(badge()?.getAttribute('aria-label')).toBe('Finish 2 of your open PRs'); + expect(badge()?.textContent).toBe('2 to finish'); + act(() => badge()?.click()); + expect(countdown()).toBe('Finish 2 PRs'); + }); + + it('stays away, badge and all, when it is off', () => { + setSettings({ finishLineMode: 'off' }); + render(many(9)); + expect(document.querySelector('[role="dialog"]')).toBeNull(); + expect(badge()).toBeNull(); + }); + + it('with the hold setting, closes only when the button is held down', () => { + vi.useFakeTimers(); + render(many(8)); + const hold = [...document.querySelectorAll('[role="dialog"] button')].find( + b => b.textContent?.includes('Hold to close') + ); + // it has focus from the start, so the keyboard can hold it right away + expect(document.activeElement).toBe(hold); + const key = (type: string, k = 'Enter') => + act(() => hold?.dispatchEvent(new KeyboardEvent(type, { key: k, bubbles: true }))); + // a tap of Enter does nothing but say to hold it; neither does Esc + key('keydown'); + act(() => vi.advanceTimersByTime(200)); + key('keyup'); + expect(hold?.textContent).toBe('Hold it down'); + act(() => vi.advanceTimersByTime(3000)); + act(() => document.dispatchEvent(new KeyboardEvent('keydown', { key: 'Escape' }))); + expect(dialog()).not.toBeNull(); + // holding Space the full two seconds turns the key, then it opens + key('keydown', ' '); + act(() => vi.advanceTimersByTime(2000)); + act(() => vi.advanceTimersByTime(700)); + expect(dialog()).toBeNull(); + }); + + it('slides a merged PR out of the list as the countdown ticks', () => { + vi.useFakeTimers(); + const nine = many(9); + render(nine); + expect(countdown()).toBe('Finish 2 PRs'); + render(nine.slice(1)); + expect(countdown()).toBe('Finish 1 PR'); + // still there for the slide, then gone + expect(document.querySelectorAll('.finish-row-leave')).toHaveLength(1); + expect(listed()).toBe(9); + act(() => vi.advanceTimersByTime(450)); + expect(listed()).toBe(8); + }); + + 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, { 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(3); + expect(text).toContain( + `• ${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 close the hold with a lone click', () => { + render(many(8)); + const hold = [...document.querySelectorAll('[role="dialog"] button')].find( + b => b.textContent?.includes('Hold to close') + ); + // a mouse click carries detail 1 and does nothing + act(() => hold?.dispatchEvent(new MouseEvent('click', { bubbles: true, detail: 1 }))); + expect(dialog()).not.toBeNull(); + // assistive tech sends a click with no pointer or key behind it + act(() => hold?.dispatchEvent(new MouseEvent('click', { bubbles: true, detail: 0 }))); + expect(dialog()).toBeNull(); + }); + + it('with the hold setting, keeps Esc from closing it or reaching anything behind it', () => { + render(many(8)); + let reached = false; + const behind = (e: KeyboardEvent) => { + if (!e.defaultPrevented) reached = true; + }; + document.addEventListener('keydown', behind); + act(() => { + document.body.dispatchEvent( + new KeyboardEvent('keydown', { + key: 'Escape', + bubbles: true, + cancelable: true, + }) + ); + }); + document.removeEventListener('keydown', behind); + expect(reached).toBe(false); + expect(dialog()).not.toBeNull(); + }); + + it('zeroes the last showing once you are under the limits, but keeps its time', () => { + vi.useFakeTimers(); + const nine = many(9); + render(nine); + const shownAt = JSON.parse(localStorage.getItem('pd2.finishLine') ?? 'null').at; + render(nine.slice(0, 3)); + expect(JSON.parse(localStorage.getItem('pd2.finishLine') ?? 'null')).toEqual({ + at: shownAt, + count: 0, + over: [], + }); + }); + + it('going back over within 4 hours waits out the rest of them', () => { + const nine = many(9); + render(nine); + act(() => + document + .querySelector('[data-hold]') + ?.dispatchEvent(new MouseEvent('click', { bubbles: true, detail: 0 })) + ); + expect(dialog()).toBeNull(); + render(nine.slice(0, 3)); // under: the record zeroes + render([...nine, pull()]); // over again, minutes later + expect(dialog()).toBeNull(); + }); + + it('board hotkeys stand down while it is open', async () => { + const { modalOpen } = await import('../hooks'); + render(many(8)); + expect(modalOpen()).toBe(true); + }); + + it('opens the preview every time Settings asks, or the URL says #finish=1', () => { + render(many(2)); + expect(dialog()).toBeNull(); + act(() => window.dispatchEvent(new Event('pd2:preview-finish-line'))); + expect(dialog()).not.toBeNull(); + act(() => + document + .querySelector('[data-hold]') + ?.dispatchEvent(new MouseEvent('click', { bubbles: true, detail: 0 })) + ); + expect(dialog()).toBeNull(); + act(() => { + location.hash = 'finish=1'; + window.dispatchEvent(new HashChangeEvent('hashchange')); + }); + expect(dialog()).not.toBeNull(); + location.hash = ''; + }); + + it('a quick Space tap is not a lone click, even where the browser clicks on keyup', () => { + render(many(8)); + const hold = [...document.querySelectorAll('[role="dialog"] button')].find( + b => b.textContent?.includes('Hold to close') + ); + act(() => { + hold?.dispatchEvent(new KeyboardEvent('keydown', { key: ' ', bubbles: true })); + hold?.dispatchEvent(new KeyboardEvent('keyup', { key: ' ', bubbles: true })); + // Firefox's keyup click, with no pointer behind it + hold?.dispatchEvent(new MouseEvent('click', { bubbles: true, detail: 0 })); + }); + expect(dialog()).not.toBeNull(); + }); + + it('keeps the last showing while it doesn’t know who you are', () => { + localStorage.setItem( + 'pd2.finishLine', + JSON.stringify({ at: Date.now(), count: 9, over: [] }) + ); + const r = (root ??= createRoot(host)); + act(() => r.render()); + expect(localStorage.getItem('pd2.finishLine')).not.toBeNull(); + expect(dialog()).toBeNull(); + }); + + it('stays down when another window showed it a moment ago', () => { + // another window wrote the record after this one loaded + const r = (root ??= createRoot(host)); + act(() => + r.render() + ); + localStorage.setItem( + 'pd2.finishLine', + JSON.stringify({ at: Date.now(), count: 8, over: [] }) + ); + act(() => r.render()); + expect(dialog()).toBeNull(); + }); + + it('waits while another dialog is open', () => { + const other = document.createElement('div'); + other.setAttribute('aria-modal', 'true'); + document.body.append(other); + render(many(9)); + expect(dialog()).toBeNull(); + other.remove(); + }); + + it('a drop ends a drag even when the dragged row left the page', () => { + const row = document.createElement('div'); + document.body.append(row); + row.dispatchEvent(new Event('dragstart', { bubbles: true })); + row.remove(); // moved to another lane: its dragend never reaches the page + document.body.dispatchEvent(new Event('drop', { bubbles: true })); + render(many(9)); + expect(dialog()).not.toBeNull(); + }); + + it('closes on Esc or the close button, and stays away until things get worse', () => { + setSettings({ finishLineHoldToClose: false }); + const nine = many(9); + render(nine); + // focus starts on the dialog itself, so a stray Enter can't close it; + // there's no hold button + expect(document.activeElement).toBe(dialog()); + expect(document.querySelector('[data-close]')?.getAttribute('aria-label')).toBe('Close'); + expect(document.querySelector('[data-hold]')).toBeNull(); + esc(); + expect(dialog()).toBeNull(); + render([...nine, pull()]); // worse, minutes later: within the 4 hours + expect(dialog()).toBeNull(); + act(() => badge()?.click()); + expect(dialog()).not.toBeNull(); + act(() => document.querySelector('[data-close]')?.click()); + expect(dialog()).toBeNull(); + }); + + it('the cog closes it and opens Settings at its own settings', () => { + setSettings({ finishLineHoldToClose: false }); + const asked: unknown[] = []; + const listen = (e: Event) => asked.push((e as CustomEvent).detail); + window.addEventListener('pd2:open-settings', listen); + render(many(9)); + act(() => + document.querySelector('[aria-label="Finish line settings"]')?.click() + ); + window.removeEventListener('pd2:open-settings', listen); + expect(asked).toEqual(['finish-line']); + expect(dialog()).toBeNull(); + }); +}); + +describe('FinishLine stats', () => { + beforeEach(() => setSettings({ finishLineHoldToClose: false })); + + it('shows only the numbers above zero, old PRs alone when the count is fine', () => { + render([pull(20), pull(30), pull(1)]); + expect(countdown()).toBe('Finish 2 PRs'); + expect(stats()).toEqual(['2 PRs older than 14 days']); + // the list is the old ones only + expect(listed()).toBe(2); + }); + + it('leaves waiting on others out of the stats', () => { + render([pull(2, { asked: ['bob'] }), ...many(7)]); + expect(headers()).toEqual(['Your move', 'Waiting on others']); + expect(stats()).toEqual(['8 open PRs, aim for 7']); + }); + + it('jumps to and focuses the first PR a stat counts', () => { + const scrolled = vi.fn(); + Element.prototype.scrollIntoView = scrolled; + const ready = pull(2, { ready: true }); + const old = pull(20, { asked: ['bob'] }); + render([ready, old, ...many(7)]); + expect(stats()).toEqual([ + '9 open PRs, aim for 7', + '1 PR older than 14 days', + '1 PR ready to merge', + ]); + const row = (p: DerivedPull) => + document.querySelector(`[role="dialog"] li[data-pull="${p.data.repo}#${p.data.number}"]`); + // the old PR waits on bob, at the bottom of the list + act(() => stat('1 PR older than 14 days')?.click()); + expect(scrolled.mock.contexts.at(-1)).toBe(row(old)); + expect(document.activeElement).toBe(row(old)?.querySelector('a')); + act(() => stat('1 PR ready to merge')?.click()); + expect(document.activeElement).toBe(row(ready)?.querySelector('a')); + // the open count counts them all: the first row in the list + act(() => (document.activeElement as HTMLElement).blur()); + act(() => stat('9 open PRs, aim for 7')?.click()); + expect(document.activeElement).toBe(document.querySelector('[role="dialog"] li a')); + }); +}); + +describe('FinishLine focus and preview', () => { + beforeEach(() => setSettings({ finishLineHoldToClose: false })); + + const tab = (shiftKey = false) => { + const e = new KeyboardEvent('keydown', { + key: 'Tab', + shiftKey, + bubbles: true, + cancelable: true, + }); + act(() => { + document.activeElement?.dispatchEvent(e); + }); + return e; + }; + + it('wraps Tab past a row sliding out, so focus never leaves the list', () => { + const nine = many(9); + render(nine); + // the first PR merges: its row slides out at the end of the list + render(nine.slice(1)); + const leaving = document.querySelector('.finish-row-leave'); + const links = [...document.querySelectorAll('[role="dialog"] a[href]')]; + expect(links.at(-1)?.closest('li')).toBe(leaving); + const lastLive = links.at(-2); + act(() => lastLive?.focus()); + expect(tab().defaultPrevented).toBe(true); + const first = document.querySelector('[role="dialog"] button'); + expect(document.activeElement).toBe(first); + // and back the other way + expect(tab(true).defaultPrevented).toBe(true); + expect(document.activeElement).toBe(lastLive); + }); + + it('sends Tab from the dialog itself to either end', () => { + render(many(9)); + const root = dialog() as HTMLElement; + const reach = [...root.querySelectorAll('a[href], button')]; + act(() => root.focus()); + expect(tab(true).defaultPrevented).toBe(true); + expect(document.activeElement).toBe(reach.at(-1)); + act(() => root.focus()); + expect(tab().defaultPrevented).toBe(true); + expect(document.activeElement).toBe(reach[0]); + }); + + it('skips anything hidden when wrapping', () => { + render(many(9)); + const links = [...document.querySelectorAll('[role="dialog"] a[href]')]; + links.at(-1)?.closest('li')?.setAttribute('hidden', ''); + act(() => links.at(-2)?.focus()); + expect(tab().defaultPrevented).toBe(true); + expect(document.activeElement).toBe(document.querySelector('[role="dialog"] button')); + }); + + it('previews your real numbers even with it off', () => { + setSettings({ finishLineMode: 'off' }); + render([pull(20), ...many(8)]); + expect(badge()).toBeNull(); + act(() => window.dispatchEvent(new Event('pd2:preview-finish-line'))); + expect(countdown()).toBe('Finish 2 PRs'); + expect(stats()).toEqual(['9 open PRs, aim for 7', '1 PR older than 14 days']); + }); + + it('previews a sample only when you are under the limits', () => { + setSettings({ finishLineMode: 'off' }); + render(many(2)); + act(() => window.dispatchEvent(new Event('pd2:preview-finish-line'))); + expect(countdown()).toBe('Nothing to finish'); + expect(stats()).toEqual([]); + expect(listed()).toBe(2); + // nothing to describe it with, so it points at nothing + expect(dialog()?.hasAttribute('aria-describedby')).toBe(false); + }); +}); + +describe('Settings, opened from the cog', () => { + it('scrolls to and focuses the Finish line settings', async () => { + const { Settings } = await import('./Settings'); + const scrolled = vi.fn(); + Element.prototype.scrollIntoView = scrolled; + const r = (root ??= createRoot(host)); + act(() => r.render( undefined} />)); + act(() => { + window.dispatchEvent(new CustomEvent('pd2:open-settings', { detail: 'finish-line' })); + }); + const section = document.getElementById('finish-line'); + expect(scrolled.mock.contexts[0]).toBe(section); + expect(section?.contains(document.activeElement)).toBe(true); + expect(document.activeElement?.textContent).toBe('Pop up'); + }); +}); diff --git a/frontend-v2/src/components/JailMode.tsx b/frontend-v2/src/components/FinishLine.tsx similarity index 58% rename from frontend-v2/src/components/JailMode.tsx rename to frontend-v2/src/components/FinishLine.tsx index 45926b14..14b7a55b 100644 --- a/frontend-v2/src/components/JailMode.tsx +++ b/frontend-v2/src/components/FinishLine.tsx @@ -1,38 +1,43 @@ import { useEffect, useMemo, useRef, useState } from 'react'; import { createPortal } from 'react-dom'; -import { Lock } from 'lucide-react'; +import { Flag, Settings as SettingsIcon, X } from 'lucide-react'; import type { DerivedPull } from '../../../shared/model/status'; import { githubUrl } from '../../../shared/format'; import { askedOf, askForInputHint, rowNote, rowWord } from '../model/actions'; import { STAGE_WORDS } from '../model/stage'; import { - jailCase, - jailDue, - jailGroups, - jailRecord, - parseJailRecord, - JAIL_KEY, - type JailCase, -} from '../model/jail'; + finishLineCase, + finishDue, + finishGroups, + finishRecord, + finishStats, + parseFinishRecord, + prCount, + FINISH_LINE_KEY, + type FinishLineCase, + type FinishStat, +} from '../model/finishLine'; import { useSettings } from '../settings'; import { readStorage, writeStorage } from '../storage'; -import { HeaderIconButton, QuietButton, RepoRef } from './bits'; +import { FactLink, QuietButton, RepoRef } from './bits'; +import { Icon } from './Icon'; import { modalOpen } from '../hooks'; import { GroupHeader } from './Lane'; import { CiGlyph } from './pips'; /** - * PR jail: when your own open PRs pass a limit (model/jail.ts), a modal pops - * up listing them, and comes back only when that gets worse (jailDue). It - * has no close button and Esc does nothing: you hold Snooze down for a - * couple of seconds (a little work on purpose), or get under the limits and - * it lets you go. While you're over, a header badge counts down what's left - * and reopens it. `#jail=1` in the URL forces it, any time, for a preview. - * The jailMode setting picks: pop up (auto), the badge alone (manual), or off. + * Finish line: when your own open PRs pass a limit (model/finishLine.ts), a + * header badge counts down how many to merge or close, and opens a list of + * them. Only you see it. Turned on in Settings, the list also pops up by + * itself, and comes back only when that gets worse (finishDue). Esc or the + * close button puts it away; finishLineHoldToClose makes you hold a button + * down instead. Getting under the limits closes it too. `#finish=1` in the + * URL forces it, any time, for a preview. The finishLineMode setting picks: + * pop up (auto), the badge alone (manual), or off. */ // whether a mouse button or finger is down, or a native drag is under way: -// the jail never pops up mid-drag (a Roadmap bar or row, say) +// it never pops up mid-drag (a Roadmap bar or row, say) let pressing = false; let dragging = false; const pointerBusy = () => pressing || dragging; @@ -60,10 +65,10 @@ if (typeof document !== 'undefined') { window.addEventListener('blur', () => (pressing = dragging = false)); } -const hasPreviewFlag = () => new URLSearchParams(location.hash.slice(1)).get('jail') === '1'; +const hasPreviewFlag = () => new URLSearchParams(location.hash.slice(1)).get('finish') === '1'; /** Renders the header badge in place, and the cell over the page. */ -export function JailMode({ +export function FinishLine({ pulls, me, extraBots, @@ -78,56 +83,59 @@ export function JailMode({ // read on the first render: the app rewrites the hash right after and // drops the unknown param const [preview, setPreview] = useState(hasPreviewFlag); - // which jail is open, not a copy of it: the list below is read live, so a + // which list is open, not a copy of it: the list below is read live, so a // merge or a new PR shows up while it's open const [shown, setShown] = useState<'real' | 'preview' | null>(null); // the last showing, kept here too so blocked storage still holds it for // this page load; another tab's showing arrives by the storage event - const record = useRef(parseJailRecord(readStorage(JAIL_KEY))); + const record = useRef(parseFinishRecord(readStorage(FINISH_LINE_KEY))); - const found = useMemo( + // where you stand whatever the mode, for the preview: with it off, a + // preview still shows your real numbers + const real = useMemo( () => // no case until we know who you are: an empty `me` (a failed whoami) // would read as "under the limits" and wipe the last showing - initialized && me && settings.jailMode !== 'off' - ? jailCase(pulls, me, extraBots, { - maxOpen: settings.jailMaxOpen, - maxDays: settings.jailMaxDays, - countDrafts: settings.jailCountDrafts, + initialized && me + ? finishLineCase(pulls, me, extraBots, { + maxOpen: settings.finishLineMaxOpen, + maxDays: settings.finishLineMaxDays, + countDrafts: settings.finishLineCountDrafts, }) : null, [initialized, pulls, me, extraBots, settings] ); + const found = settings.finishLineMode === 'off' ? null : real; // under the limits again: zero what the last showing counted, so going // over later is worse than it (a fresh offense), but keep when it showed, // so it still comes back at most every 4 hours useEffect(() => { const last = record.current; - if (!initialized || !me || found || settings.jailMode === 'off' || !last) return; + if (!initialized || !me || found || settings.finishLineMode === 'off' || !last) return; if (!last.count && !last.over.length) return; record.current = { at: last.at, count: 0, over: [] }; - writeStorage(JAIL_KEY, JSON.stringify(record.current)); - }, [initialized, me, found, settings.jailMode]); + writeStorage(FINISH_LINE_KEY, JSON.stringify(record.current)); + }, [initialized, me, found, settings.finishLineMode]); // the latest case, for closing: the button's hold started seconds ago, and // what you saw last is what the next showing compares against const latest = useRef(found); latest.current = found; - // open PRs on the whole board, so a row leaving the jail's list only slides + // open PRs on the whole board, so a row leaving the list only slides // out as merged when it really closed (not when the rule switched) const openIds = useMemo( () => new Set(pulls.filter(p => p.data.state === 'open').map(pullId)), [pulls] ); - const remember = (jail: JailCase) => { - record.current = jailRecord(jail, Date.now()); - writeStorage(JAIL_KEY, JSON.stringify(record.current)); + const remember = (c: FinishLineCase) => { + record.current = finishRecord(c, Date.now()); + writeStorage(FINISH_LINE_KEY, JSON.stringify(record.current)); }; - const show = (jail: JailCase) => { - remember(jail); + const show = (c: FinishLineCase) => { + remember(c); setShown('real'); }; @@ -135,18 +143,18 @@ export function JailMode({ const onHash = () => { if (hasPreviewFlag()) setPreview(true); }; - // Settings' "Preview PR jail": an event, since setting the same hash + // Settings' "Preview it": an event, since setting the same hash // twice fires nothing const onPreview = () => setPreview(true); - window.addEventListener('pd2:preview-jail', onPreview); + window.addEventListener('pd2:preview-finish-line', onPreview); const onStorage = (e: StorageEvent) => { - if (e.key === JAIL_KEY) record.current = parseJailRecord(e.newValue); + if (e.key === FINISH_LINE_KEY) record.current = parseFinishRecord(e.newValue); }; window.addEventListener('hashchange', onHash); window.addEventListener('storage', onStorage); return () => { window.removeEventListener('hashchange', onHash); - window.removeEventListener('pd2:preview-jail', onPreview); + window.removeEventListener('pd2:preview-finish-line', onPreview); window.removeEventListener('storage', onStorage); }; }, []); @@ -162,17 +170,17 @@ export function JailMode({ // hidden tab or a focused field waits for the next one, so it never lands // mid-typing or out of sight useEffect(() => { - if (!found || shown || settings.jailMode !== 'auto') return; + if (!found || shown || settings.finishLineMode !== 'auto') return; const check = () => { if (document.visibilityState === 'hidden') return; - // another dialog is up (Settings), or a drag is under way: the jail + // another dialog is up (Settings), or a drag is under way: it // would land on top of it, and swallow the Esc that cancels a drag if (modalOpen() || pointerBusy()) return; if (document.activeElement?.matches('input, textarea, select')) return; // read the record fresh: a second visible window may have shown it // a moment ago, before its storage event reached this one - record.current = parseJailRecord(readStorage(JAIL_KEY)) ?? record.current; - if (!jailDue(found, record.current, Date.now())) return; + record.current = parseFinishRecord(readStorage(FINISH_LINE_KEY)) ?? record.current; + if (!finishDue(found, record.current, Date.now())) return; show(found); }; check(); @@ -182,7 +190,7 @@ export function JailMode({ document.removeEventListener('visibilitychange', check); window.removeEventListener('focus', check); }; - }, [found, shown, settings.jailMode]); + }, [found, shown, settings.finishLineMode]); // you got under the limits while it was up: it says so, then lets you go const freed = shown === 'real' && !found; @@ -195,26 +203,33 @@ export function JailMode({ return ( <> {found && ( - show(found)} - className="relative" + className="pressable inline-flex h-8 items-center gap-1.5 rounded-lg border border-line bg-surface px-2.5 text-[13px] font-medium whitespace-nowrap text-ink-2 hover:text-brand" > - - {found.toGo > 9 ? '9+' : found.toGo} + + + {found.toGo} to finish - + )} {shown && createPortal( - { // what you saw last is what the next showing compares @@ -229,100 +244,136 @@ export function JailMode({ ); } -/** How long "You're out" shows before the jail closes on its own. */ +/** How long "All finished" shows before it closes on its own. */ const FREED_MS = 2500; +/** The badge's full words, for its hover and screen readers. */ +const finishLabel = (n: number) => `Finish ${n} of your open PRs`; + /** Merged or closed your way under the limits while it was up. */ -const FREE: JailCase = { +const FREE: FinishLineCase = { pulls: [], - why: 'You’re out. Nice work.', + ready: 0, count: 0, over: [], toGo: 0, }; -/** The preview when you're not in jail: your PRs as jail counts them (any +/** The preview when you're under the limits: your PRs as it counts them (any * open one trips a limit of 0), or the board's oldest when you have none, - * so there's something to look at. */ + * so there's something to look at. Nothing to finish: toGo is 0. */ function sampleCase( pulls: DerivedPull[], me: string, extraBots: ReadonlySet, - settings: { jailMaxDays: number; jailCountDrafts: boolean } -): JailCase { - const mine = jailCase(pulls, me, extraBots, { + settings: { finishLineMaxDays: number; finishLineCountDrafts: boolean } +): FinishLineCase { + const mine = finishLineCase(pulls, me, extraBots, { maxOpen: 0, - maxDays: settings.jailMaxDays, - countDrafts: settings.jailCountDrafts, + maxDays: settings.finishLineMaxDays, + countDrafts: settings.finishLineCountDrafts, }); - if (mine) return { ...mine, why: 'Preview: your open PRs, as jail would list them' }; + if (mine) return { ...mine, toGo: 0 }; return { pulls: pulls .filter(p => p.data.state === 'open') .sort((a, b) => b.ageDays - a.ageDays) .slice(0, 6), - why: 'Preview: these are sample PRs from the board', + ready: 0, count: 0, over: [], toGo: 0, }; } -/** How long "Hold to get back to work" has to be held down. */ +/** How long "Hold to close" has to be held down. */ const HOLD_MS = 2000; -/** How long the open lock shows before the jail closes. */ +/** How long the full ring shows before it closes. */ const OPEN_MS = 700; /** How long a merged or closed row takes to leave the list. */ const LEAVE_MS = 450; +const iconButton = + 'pressable inline-flex h-8 w-8 items-center justify-center rounded-lg text-ink-3 hover:bg-muted hover:text-ink'; + /** The modal over the board: a centered card on a desktop, a sheet up from - * the bottom on a phone. Its header and its one button stay put while the - * list scrolls between them. No close button and no Esc: the way out besides - * getting under the limits is holding the button down for a couple of - * seconds, by pointer, finger, Enter or Space. */ -function JailModal({ - jail, + * the bottom on a phone. Its top is the action and its numbers ("Finish 2 + * PRs", then the stats that say why); the list scrolls under it. Esc or the + * close button puts it away; with `hold` on, neither does, and the way out + * besides getting under the limits is holding the button at the bottom down + * for a couple of seconds, by pointer, finger, Enter or Space. The cog opens + * Settings at this feature's own settings. */ +function FinishModal({ + found, me, + maxOpen, maxDays, + hold, openIds, onDone, }: { - jail: JailCase; + found: FinishLineCase; me: string; + maxOpen: number; maxDays: number; + hold: boolean; openIds: ReadonlySet; onDone: () => void; }) { const rootRef = useRef(null); - const free = jail === FREE; + const free = found === FREE; + const stats = finishStats(found, { maxOpen, maxDays }); + // a stat jumps to the first PR it counts, in the list's order + const goTo = (stat: FinishStat) => { + const ids = new Set(found.pulls.filter(stat.counts).map(pullId)); + const row = [...(rootRef.current?.querySelectorAll('li[data-pull]') ?? [])].find( + li => ids.has(li.dataset.pull ?? '') + ); + row?.scrollIntoView?.({ block: 'nearest' }); + row?.querySelector('a[href]')?.focus({ preventScroll: true }); + }; + // the key handler is set up once; it reads these fresh + const latest = useRef({ hold, onDone }); + latest.current = { hold, onDone }; - // "You're out" unmounts the focused button: keep focus in the dialog + // "You're at the finish line" unmounts the focused button: keep focus in + // the dialog useEffect(() => { if (free) rootRef.current?.focus({ preventScroll: true }); }, [free]); useEffect(() => { const root = rootRef.current; - // focus goes back where it was (the header lock, say) when it closes + // focus goes back where it was (the header flag, say) when it closes const before = document.activeElement as HTMLElement | null; - // focus starts on the button, so holding Enter or Space works right - // away; the dialog itself when it's showing "You're out" - const hold = root?.querySelector('[data-hold]'); - (hold ?? root)?.focus({ preventScroll: true }); - // Esc doesn't close it (holding the button does); Tab stays inside + // focus starts on the hold button, so holding Enter or Space works + // right away; with no hold, on the dialog itself, so a stray Enter + // can't close it before it's read + const start = root?.querySelector('[data-hold]'); + (start ?? root)?.focus({ preventScroll: true }); const onKey = (e: KeyboardEvent) => { - // Esc does nothing here, and must not reach a dialog behind this one - // (Settings) either: listening in the capture phase runs first + // Esc closes it (unless you asked to hold), and must not reach a + // dialog behind this one (Settings) either: listening in the + // capture phase runs first if (e.key === 'Escape') { e.preventDefault(); e.stopPropagation(); + if (!latest.current.hold) latest.current.onDone(); } if (e.key !== 'Tab' || !root) return; - const focusables = root.querySelectorAll('a[href], button'); + // only what Tab can reach: not a row sliding out, nor anything + // hidden or not rendered, or the wrap below never sees the real last + const focusables = [...root.querySelectorAll('a[href], button')].filter( + el => !el.closest('[hidden], [aria-hidden="true"]') && (el.checkVisibility?.() ?? true) + ); if (!focusables.length) return; const first = focusables[0]; const last = focusables[focusables.length - 1]; - if (e.shiftKey && document.activeElement === first) { + // from the dialog itself (where it opens), Tab goes in at either end + if (document.activeElement === root) { + e.preventDefault(); + (e.shiftKey ? last : first).focus(); + } else if (e.shiftKey && document.activeElement === first) { e.preventDefault(); last.focus(); } else if (!e.shiftKey && document.activeElement === last) { @@ -349,36 +400,77 @@ function JailModal({ tabIndex={-1} role="dialog" aria-modal="true" - aria-labelledby="jail-title" - aria-describedby="jail-why" - className="fixed inset-0 z-[200] flex flex-col items-center justify-end bg-ink/40 pt-8 outline-none sm:justify-start sm:px-4 sm:pt-[8vh] sm:pb-8" + aria-labelledby="finish-title" + aria-describedby={free ? 'finish-done' : stats.length ? 'finish-stats' : undefined} + className="fixed inset-0 z-[200] flex flex-col items-center justify-end bg-black/30 pt-8 outline-none sm:justify-start sm:px-4 sm:pt-[8vh] sm:pb-8" >
-
- +
+ {free && }
+ {/* the action with its real number, ticking down live as + you merge or close */}

- PR jail - {/* the countdown to freedom: ticks down live as you merge - or close */} - - {free ? 'Free' : `${jail.toGo} to go`} - + {free + ? 'All finished' + : found.toGo + ? `Finish ${prCount(found.toGo)}` + : 'Nothing to finish'}

-

+ None of your open PRs are past your limits. +

+ )} + {/* why, in numbers you can act on: each one jumps to its PRs */} + {!free && stats.length > 0 && ( +

+ {/* spaced, not dotted, so a wrap never leaves a dot hanging */} + {stats.map(stat => ( + goTo(stat)}> + {stat.text} + + ))} +

+ )} +
+
+ + {!hold && ( + + )}
{!free && ( @@ -387,9 +479,10 @@ function JailModal({ of this list on the card's color instead of under the app header on the canvas */}
- +
- +

Only you see this.

+ {hold && } )}
@@ -397,12 +490,10 @@ function JailModal({ ); } -/** The one button: hold it down for HOLD_MS, by pointer, finger, Enter or - * Space. The key in the lock turns a quarter turn as you hold and the ring - * around it fills, both off the same clock; letting go early springs them - * back, and a quick click says to hold it. At the end the lock opens and - * the jail closes. */ -function HoldToLeave({ onDone }: { onDone: () => void }) { +/** The hold button: hold it down for HOLD_MS, by pointer, finger, Enter or + * Space. The ring fills as you hold; letting go early springs it back, and a + * quick click says to hold it. At the end it closes. */ +function HoldToClose({ onDone }: { onDone: () => void }) { const [holding, setHolding] = useState(false); const [opened, setOpened] = useState(false); // a click too short to be a hold: the label teaches the gesture @@ -442,8 +533,7 @@ function HoldToLeave({ onDone }: { onDone: () => void }) { [] ); const isPress = (key: string) => key === 'Enter' || key === ' '; - // the ring and the key share one timing: fill and turn while held, snap - // back when let go + // the ring fills while held, and snaps back when let go const sweep = holding ? `${HOLD_MS}ms linear` : '150ms ease-out'; return ( ); @@ -621,7 +684,7 @@ function CopyNudge({ rows }: { rows: { pull: DerivedPull }[] }) { * quickest way out first, each group under the board's own header with a * word on what to do, oldest first. Someone else's PRs (the preview's * fallback) list flat, since their groups would be a reviewer's. */ -function JailList({ +function FinishList({ pulls, me, maxDays, @@ -637,7 +700,7 @@ function JailList({ const all = [...pulls, ...leaving]; const mine = all.every(p => p.data.user.login === me); const groups = mine - ? jailGroups(all, me) + ? finishGroups(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 @@ -646,9 +709,21 @@ function JailList({ 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 }, { + key: 'ready' as const, + title: STAGE_WORDS.ready, + sub: 'Merge these first', + rows: groups.ready, + ready: true, + }, + { + key: 'move' as const, + title: 'Your move', + sub: 'Do the next step on each', + rows: groups.move, + }, + { + key: 'waiting' as const, title: 'Waiting on others', sub: 'Nudge whoever you asked, or close what you don’t need', rows: groups.waiting, @@ -658,7 +733,7 @@ function JailList({ return (
{sections.map(g => ( -
+
{mine && ( {g.rows.map(({ pull }) => ( - maxDays; return (
  • @@ -728,9 +804,12 @@ function JailRow({ > {p.data.title} ↗ + {/* no amber past the limit: the stat above already says how many + are, and amber on every row would stop meaning "look here"; + weight alone sets the old ones apart */} @@ -759,9 +838,9 @@ function JailRow({ ); } -/** The inmate: a sad robot behind bars, drawn in the board's own line - * weights and color tokens so it themes with everything else. */ -function SadRobot() { +/** A robot cheering at a checkered finish flag, drawn in the board's own + * line weights and color tokens so it themes with everything else. */ +function FinishMark() { return ( - {/* a drooping antenna */} - - - {/* head, and the face on its screen */} + {/* the finish line on the ground */} + + {/* the flag: a pole and a checkered cloth */} + + + + {/* an upright antenna with a lit tip */} + + + {/* head, and a happy face on its screen */} - - - - - {/* striped prison body */} + + + {/* body, one arm up at the flag, one down */} - - {/* the bars, in front of him */} + ); } diff --git a/frontend-v2/src/components/JailMode.test.tsx b/frontend-v2/src/components/JailMode.test.tsx deleted file mode 100644 index 94bac16d..00000000 --- a/frontend-v2/src/components/JailMode.test.tsx +++ /dev/null @@ -1,331 +0,0 @@ -// @vitest-environment jsdom -import { act } from 'react'; -import { createRoot, type Root } from 'react-dom/client'; -import { afterEach, describe, expect, it, vi } from 'vitest'; -import { derive, type DerivedPull } from '../../../shared/model/status'; -import type { PullData } from '../../../shared/types'; -import { setSettings } from '../settings'; -import { JailMode } from './JailMode'; - -// React's act() warns unless the environment says it's a test -(globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true; - -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 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 = { - repo: 'iFixit/ifixit', - number: n, - state: 'open', - title: `PR ${n}`, - body: '', - draft: false, - created_at: at, - updated_at: at, - closed_at: null, - merged_at: null, - mergeable: true, - difficulty: null, - additions: 10, - deletions: 5, - changed_files: 2, - milestone: { title: null, due_on: null }, - head: { ref: 'branch', sha: 'abc', repo: { owner: { login: 'iFixit' } } }, - base: { ref: 'master' }, - user: { login: 'me' }, - status: { - cr_req: ready ? 0 : 1, - qa_req: ready ? 0 : 1, - allCR: [], - allQA: [], - dev_block: [], - deploy_block: [], - commit_statuses: [], - }, - 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); -}; -const many = (count: number) => Array.from({ length: count }, () => pull()); - -let root: Root | null = null; -const host = document.createElement('div'); -document.body.append(host); -afterEach(() => { - act(() => root?.unmount()); - root = null; - localStorage.clear(); - setSettings({ jailMode: 'auto' }); - vi.useRealTimers(); -}); - -function render(pulls: DerivedPull[]) { - const r = (root ??= createRoot(host)); - act(() => r.render()); -} - -const why = () => document.getElementById('jail-why')?.textContent; -const listed = () => document.querySelectorAll('[role="dialog"] li').length; -const dialog = () => document.querySelector('[role="dialog"]'); -const countdown = () => document.querySelector('#jail-title span')?.textContent; -const headers = () => [...document.querySelectorAll('[role="dialog"] h3')].map(h => h.textContent); - -describe('JailMode while the cell is down', () => { - it('follows new PRs and merges live', () => { - const first = many(8); - render(first); - expect(why()).toMatch(/^8 open PRs/); - expect(listed()).toBe(8); - - // a new PR arrives over the socket - render([...first, pull()]); - expect(why()).toMatch(/^9 open PRs/); - expect(listed()).toBe(9); - - expect(countdown()).toBe('2 to go'); - - // merges bring you under the limits: it says so, then lets you go - vi.useFakeTimers(); - render(first.slice(0, 3)); - expect(why()).toBe('You’re out. Nice work.'); - expect(countdown()).toBe('Free'); - expect(listed()).toBe(0); - act(() => vi.advanceTimersByTime(2500)); - expect(dialog()).toBeNull(); - }); - - it('lifts ready PRs to the top and counts them in the why line', () => { - const ready = pull(2, { ready: true }); - render([...many(7), ready]); - // 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 - ); - expect(why()).toBe('8 open PRs. Parole at 7. Merge your ready one and you’re out.'); - }); - - it('only shows the badge when PR jail is manual', () => { - setSettings({ jailMode: 'manual' }); - render(many(9)); - expect(document.querySelector('[role="dialog"]')).toBeNull(); - const badge = document.querySelector('[aria-label^="PR jail"]'); - expect(badge).not.toBeNull(); - act(() => badge?.click()); - expect(why()).toMatch(/^9 open PRs/); - }); - - it('stays away, badge and all, when PR jail is off', () => { - setSettings({ jailMode: 'off' }); - render(many(9)); - expect(document.querySelector('[role="dialog"]')).toBeNull(); - expect(document.querySelector('[aria-label^="PR jail"]')).toBeNull(); - }); - - it('lets you out only when the button is held down, by key or pointer', () => { - vi.useFakeTimers(); - render(many(8)); - const snooze = [ - ...document.querySelectorAll('[role="dialog"] button'), - ].find(b => b.textContent?.includes('Hold to get back to work')); - // it has focus from the start, so the keyboard can hold it right away - expect(document.activeElement).toBe(snooze); - const key = (type: string, k = 'Enter') => - act(() => snooze?.dispatchEvent(new KeyboardEvent(type, { key: k, bubbles: true }))); - // a tap of Enter does nothing but say to hold it; neither does Esc - key('keydown'); - act(() => vi.advanceTimersByTime(200)); - key('keyup'); - expect(snooze?.textContent).toBe('Hold it down'); - act(() => vi.advanceTimersByTime(3000)); - act(() => document.dispatchEvent(new KeyboardEvent('keydown', { key: 'Escape' }))); - expect(dialog()).not.toBeNull(); - // holding Space the full two seconds turns the key, then it opens - key('keydown', ' '); - act(() => vi.advanceTimersByTime(2000)); - act(() => vi.advanceTimersByTime(700)); - expect(dialog()).toBeNull(); - }); - - it('slides a merged PR out of the list as the countdown ticks', () => { - vi.useFakeTimers(); - const nine = many(9); - render(nine); - expect(countdown()).toBe('2 to go'); - render(nine.slice(1)); - expect(countdown()).toBe('1 to go'); - // still there for the slide, then gone - expect(document.querySelectorAll('.jail-row-leave')).toHaveLength(1); - expect(listed()).toBe(9); - act(() => vi.advanceTimersByTime(450)); - expect(listed()).toBe(8); - }); - - 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, { 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(3); - expect(text).toContain( - `• ${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( - b => b.textContent?.includes('Hold to get back to work') - ); - // a mouse click carries detail 1 and does nothing - act(() => hold?.dispatchEvent(new MouseEvent('click', { bubbles: true, detail: 1 }))); - expect(dialog()).not.toBeNull(); - // assistive tech sends a click with no pointer or key behind it - act(() => hold?.dispatchEvent(new MouseEvent('click', { bubbles: true, detail: 0 }))); - expect(dialog()).toBeNull(); - }); - - it('keeps Esc from reaching anything behind it', () => { - render(many(8)); - let reached = false; - const behind = (e: KeyboardEvent) => { - if (!e.defaultPrevented) reached = true; - }; - document.addEventListener('keydown', behind); - act(() => { - document.body.dispatchEvent( - new KeyboardEvent('keydown', { key: 'Escape', bubbles: true, cancelable: true }) - ); - }); - document.removeEventListener('keydown', behind); - expect(reached).toBe(false); - expect(dialog()).not.toBeNull(); - }); - - it('zeroes the last showing once you are under the limits, but keeps its time', () => { - vi.useFakeTimers(); - const nine = many(9); - render(nine); - const shownAt = JSON.parse(localStorage.getItem('pd2.jail') ?? 'null').at; - render(nine.slice(0, 3)); - expect(JSON.parse(localStorage.getItem('pd2.jail') ?? 'null')).toEqual({ - at: shownAt, - count: 0, - over: [], - }); - }); - - it('going back over within 4 hours waits out the rest of them', () => { - const nine = many(9); - render(nine); - act(() => - document - .querySelector('[data-hold]') - ?.dispatchEvent(new MouseEvent('click', { bubbles: true, detail: 0 })) - ); - expect(dialog()).toBeNull(); - render(nine.slice(0, 3)); // under: the record zeroes - render([...nine, pull()]); // over again, minutes later - expect(dialog()).toBeNull(); - }); - - it('board hotkeys stand down while it is open', async () => { - const { modalOpen } = await import('../hooks'); - render(many(8)); - expect(modalOpen()).toBe(true); - }); - - it('opens the preview every time Settings asks', () => { - render(many(2)); - expect(dialog()).toBeNull(); - act(() => window.dispatchEvent(new Event('pd2:preview-jail'))); - expect(dialog()).not.toBeNull(); - }); - - it('a quick Space tap is not a lone click, even where the browser clicks on keyup', () => { - render(many(8)); - const hold = [...document.querySelectorAll('[role="dialog"] button')].find( - b => b.textContent?.includes('Hold to get back to work') - ); - act(() => { - hold?.dispatchEvent(new KeyboardEvent('keydown', { key: ' ', bubbles: true })); - hold?.dispatchEvent(new KeyboardEvent('keyup', { key: ' ', bubbles: true })); - // Firefox's keyup click, with no pointer behind it - hold?.dispatchEvent(new MouseEvent('click', { bubbles: true, detail: 0 })); - }); - expect(dialog()).not.toBeNull(); - }); - - it('keeps the last showing while it doesn’t know who you are', () => { - localStorage.setItem('pd2.jail', JSON.stringify({ at: Date.now(), count: 9, over: [] })); - const r = (root ??= createRoot(host)); - act(() => r.render()); - expect(localStorage.getItem('pd2.jail')).not.toBeNull(); - expect(dialog()).toBeNull(); - }); - - it('stays down when another window showed it a moment ago', () => { - // another window wrote the record after this one loaded - const r = (root ??= createRoot(host)); - act(() => - r.render() - ); - localStorage.setItem('pd2.jail', JSON.stringify({ at: Date.now(), count: 8, over: [] })); - act(() => r.render()); - expect(dialog()).toBeNull(); - }); - - it('waits while another dialog is open', () => { - const other = document.createElement('div'); - other.setAttribute('aria-modal', 'true'); - document.body.append(other); - render(many(9)); - expect(dialog()).toBeNull(); - other.remove(); - }); - - it('a drop ends a drag even when the dragged row left the page', () => { - const row = document.createElement('div'); - document.body.append(row); - row.dispatchEvent(new Event('dragstart', { bubbles: true })); - row.remove(); // moved to another lane: its dragend never reaches the page - document.body.dispatchEvent(new Event('drop', { bubbles: true })); - render(many(9)); - expect(dialog()).not.toBeNull(); - }); -}); diff --git a/frontend-v2/src/components/Legend.tsx b/frontend-v2/src/components/Legend.tsx index 2a3b0695..77f36e8e 100644 --- a/frontend-v2/src/components/Legend.tsx +++ b/frontend-v2/src/components/Legend.tsx @@ -1,5 +1,5 @@ import type { ReactNode } from 'react'; -import { CircleHelp, Heart, Star } from 'lucide-react'; +import { CircleHelp, Flag, Heart, Star } from 'lucide-react'; import { HeaderIconButton } from './bits'; import { Icon } from './Icon'; import { Popover } from './Popover'; @@ -160,6 +160,24 @@ export function Legend() { /> + + + + + 2 to finish + + + } + def="shows in the header when your own open PRs pass your limits (by default more than 7 open, or one open over 14 days): how many to merge or close. Click it for the list, ready ones first. Only you see it; nobody else is told" + /> + Settings} + def="under Finish line, or the cog in the list: Pop up opens the list by itself when you go over (off by default), Header count only waits for a click, Off hides it. Esc or ✕ closes it, unless you’ve chosen to hold to close" + /> + + weight:xs,s} diff --git a/frontend-v2/src/components/Settings.tsx b/frontend-v2/src/components/Settings.tsx index 992a513a..23193859 100644 --- a/frontend-v2/src/components/Settings.tsx +++ b/frontend-v2/src/components/Settings.tsx @@ -74,7 +74,10 @@ function CodeRegionsGroup() { setDraft(e.target.value)} - onKeyDown={commitKeyHandler({ onCommit: add, onCancel: () => setDraft('') })} + onKeyDown={commitKeyHandler({ + onCommit: add, + onCancel: () => setDraft(''), + })} placeholder="e.g. Growthbook, Shopify, Diagrams" aria-label="add a code region" className="min-w-0 flex-1 rounded-md border border-line bg-surface px-2 py-1 text-sm outline-none focus-visible:border-brand" @@ -123,10 +126,19 @@ export function Settings({ const triggerRef = useRef(null); const { armed: armReset, run: runReset } = useArmedConfirm(); + // which section to scroll to and focus once open: a Group's id + const [section, setSection] = useState(null); + // open on demand — the code-regions tip's "Set them up" action dispatches - // this so a reader can jump straight here from the board + // this so a reader can jump straight here from the board; a CustomEvent + // whose detail names a Group's id opens at that group (Finish line's cog) useEffect(() => { - const openIt = () => setOpen(true); + const openIt = (e: Event) => { + setSection( + typeof (e as CustomEvent).detail === 'string' ? (e as CustomEvent).detail : null + ); + setOpen(true); + }; window.addEventListener('pd2:open-settings', openIt); return () => window.removeEventListener('pd2:open-settings', openIt); }, []); @@ -134,7 +146,7 @@ export function Settings({ useEffect(() => { if (!open) return; const onKey = (e: KeyboardEvent) => { - // a dialog above this one (PR jail) handled it + // a dialog above this one (Finish line) handled it if (e.defaultPrevented) return; if (e.key === 'Escape') setOpen(false); // aria-modal promises a focus trap; without this, Tab walks out of @@ -161,12 +173,20 @@ export function Settings({ // this blocks scroll const prevOverflow = document.body.style.overflow; document.body.style.overflow = 'hidden'; - // focus the panel so Escape and tabbing land inside it + // focus the panel so Escape and tabbing land inside it, or the first + // control of the section it was opened at panelRef.current?.focus(); + if (section) { + const target = panelRef.current?.querySelector(`#${section}`); + target?.scrollIntoView({ block: 'start' }); + target?.querySelector('button, input')?.focus({ preventScroll: true }); + setSection(null); + } return () => { document.removeEventListener('keydown', onKey); document.body.style.overflow = prevOverflow; }; + // section is read once, when it opens, so it isn't a dependency }, [open]); // return focus to the cog when the panel closes — but only after a real @@ -330,73 +350,6 @@ export function Settings({ onChange={ageDisplay => set({ ageDisplay })} /> - - set({ jailMode })} - /> - - - set({ jailMaxOpen })} - /> - - - set({ jailMaxDays })} - /> - - - set({ jailCountDrafts: v === 'on' })} - /> - - - { - // close Settings first so the jail isn't stacked on it - setOpen(false); - window.dispatchEvent(new Event('pd2:preview-jail')); - }} - > - Preview PR jail - - + + + A count in the header of your own open PRs past your limits, and a list + of them to merge or close. Only you see it. + + + set({ finishLineMode })} + /> + + + set({ finishLineMaxOpen })} + /> + + + set({ finishLineMaxDays })} + /> + + + set({ finishLineCountDrafts: v === 'on' })} + /> + + + set({ finishLineHoldToClose: v === 'on' })} + /> + + + { + // close Settings first so the list isn't stacked on it + setOpen(false); + window.dispatchEvent(new Event('pd2:preview-finish-line')); + }} + > + Preview + + + + diff --git a/frontend-v2/src/components/SettingsBits.tsx b/frontend-v2/src/components/SettingsBits.tsx index 2b84c556..cfcbda42 100644 --- a/frontend-v2/src/components/SettingsBits.tsx +++ b/frontend-v2/src/components/SettingsBits.tsx @@ -6,9 +6,22 @@ import { Icon } from './Icon'; /** A labelled group of controls inside a settings surface (the Settings panel * or the bell's nudge-settings view) — one definition so both render the * section chrome identically. */ -export function Group({ title, children }: { title: string; children: ReactNode }) { +export function Group({ + title, + id, + children, +}: { + title: string; + /** a target to open Settings at (see Settings' pd2:open-settings) */ + id?: string; + children: ReactNode; +}) { return ( -
    +

    {title}

    diff --git a/frontend-v2/src/hooks.ts b/frontend-v2/src/hooks.ts index b9cec02b..34aaeb15 100644 --- a/frontend-v2/src/hooks.ts +++ b/frontend-v2/src/hooks.ts @@ -1,6 +1,6 @@ import { type RefObject, useEffect, useLayoutEffect } from 'react'; -/** A modal dialog is open (PR jail, Settings): the board's own keys stand +/** A modal dialog is open (Finish line, Settings): the board's own keys stand * down, so `/` or `j` can't move focus behind the scrim. */ export const modalOpen = () => document.querySelector('[aria-modal="true"]') != null; diff --git a/frontend-v2/src/model/finishLine.test.ts b/frontend-v2/src/model/finishLine.test.ts new file mode 100644 index 00000000..e4008aca --- /dev/null +++ b/frontend-v2/src/model/finishLine.test.ts @@ -0,0 +1,201 @@ +import { describe, expect, it } from 'vitest'; +import { + FINISH_COOLDOWN_MS, + finishLineCase, + finishDue, + finishRecord, + finishStats, + parseFinishRecord, + type FinishPull, +} from './finishLine'; + +const LIMITS = { maxOpen: 7, maxDays: 14 }; +const NO_BOTS: ReadonlySet = new Set(); + +let n = 0; +function pull( + login: string, + ageDays: number, + o: { + draft?: boolean; + deployBlockedBy?: string[]; + ready?: boolean; + cryo?: boolean; + } = {} +): FinishPull { + n += 1; + return { + data: { + repo: 'iFixit/ifixit', + number: n, + title: `PR ${n}`, + draft: o.draft ?? false, + state: 'open', + user: { login }, + }, + ageDays, + deployBlockedBy: o.deployBlockedBy ?? [], + status: o.ready ? 'ready' : 'needs_cr', + cryo: o.cryo ?? false, + }; +} + +const many = (count: number, login = 'me', ageDays = 1) => + Array.from({ length: count }, () => pull(login, ageDays)); + +describe('finishLineCase', () => { + it('counts the ready ones it lists', () => { + const pulls = [...many(7), pull('me', 1, { ready: true }), pull('me', 2, { ready: true })]; + expect(finishLineCase(pulls, 'me', NO_BOTS, LIMITS)?.ready).toBe(2); + }); + + it('never counts a parked PR as ready', () => { + const parked = pull('me', 1, { ready: true, cryo: true }); + expect(finishLineCase([...many(7), parked], 'me', NO_BOTS, LIMITS)?.ready).toBe(0); + }); + + it('counts what’s left to close, the old ones included', () => { + // 9 open (2 over the limit) and 3 old: closing the 3 old ones frees you + const pulls = [...many(6), pull('me', 20), pull('me', 30), pull('me', 40)]; + expect(finishLineCase(pulls, 'me', NO_BOTS, LIMITS)?.toGo).toBe(3); + // 12 open (5 over) and 1 old: 5 to go, the old one among them + expect(finishLineCase([...many(11), pull('me', 20)], 'me', NO_BOTS, LIMITS)?.toGo).toBe(5); + }); + + it('stays free at the open-PR limit', () => { + expect(finishLineCase(many(7), 'me', NO_BOTS, LIMITS)).toBeNull(); + }); + + it('counts you over past the open-PR limit, listing them oldest first', () => { + const pulls = [...many(7), pull('me', 9)]; + const c = finishLineCase(pulls, 'me', NO_BOTS, LIMITS); + expect(c?.pulls).toHaveLength(8); + expect(c?.pulls[0].ageDays).toBe(9); + expect(c?.count).toBe(8); + expect(c?.toGo).toBe(1); + }); + + it('counts you over for one PR past the age limit, and lists only the old ones', () => { + const old = pull('me', 21); + const c = finishLineCase([pull('me', 2), old, pull('me', 14)], 'me', NO_BOTS, LIMITS); + expect(c?.pulls).toEqual([old]); + expect(c?.count).toBe(3); + expect(c?.over).toEqual([`iFixit/ifixit#${old.data.number}`]); + expect(c?.toGo).toBe(1); + }); + + it('ignores other people’s PRs', () => { + expect( + finishLineCase([...many(9, 'someone'), pull('someone', 90)], 'me', NO_BOTS, LIMITS) + ).toBeNull(); + }); + + it('ignores drafts unless they count', () => { + const drafts = Array.from({ length: 9 }, () => pull('me', 90, { draft: true })); + expect(finishLineCase(drafts, 'me', NO_BOTS, LIMITS)).toBeNull(); + const counted = finishLineCase(drafts, 'me', NO_BOTS, { + ...LIMITS, + countDrafts: true, + }); + expect(counted?.pulls).toHaveLength(9); + }); + + it('ignores bots, by suffix and by the config list', () => { + expect( + finishLineCase(many(9, 'dependabot[bot]'), 'dependabot[bot]', NO_BOTS, LIMITS) + ).toBeNull(); + expect( + finishLineCase(many(9, 'renovate'), 'renovate', new Set(['renovate']), LIMITS) + ).toBeNull(); + }); + + it('ignores PRs held for a deploy', () => { + const held = Array.from({ length: 9 }, () => pull('me', 90, { deployBlockedBy: ['qa'] })); + expect(finishLineCase(held, 'me', NO_BOTS, LIMITS)).toBeNull(); + }); +}); + +describe('finishDue', () => { + const HOUR = 60 * 60 * 1000; + const T = 1_000_000_000_000; + const over = (pulls: FinishPull[]) => finishLineCase(pulls, 'me', NO_BOTS, LIMITS); + const recordAt = (pulls: FinishPull[], at: number) => { + const c = over(pulls); + if (!c) throw new Error('expected to be over a limit'); + return finishRecord(c, at); + }; + const eight = many(8); + const shownAt = recordAt(eight, T); + + it('never shows while you are under the limits', () => { + expect(finishDue(over(many(7)), null, T)).toBe(false); + }); + + it('shows the first time', () => { + expect(finishDue(over(eight), null, T)).toBe(true); + }); + + it('does not re-show on a refresh', () => { + expect(finishDue(over(eight), shownAt, T + 1000)).toBe(false); + }); + + it('does not re-show within 4 hours, even when it got worse', () => { + expect(finishDue(over([...eight, pull('me', 1)]), shownAt, T + 3 * HOUR)).toBe(false); + }); + + it('re-shows after 4 hours when you have more open PRs', () => { + expect(finishDue(over([...eight, pull('me', 1)]), shownAt, T + FINISH_COOLDOWN_MS)).toBe( + true + ); + }); + + it('does not re-show after 4 hours when it is the same or better', () => { + expect(finishDue(over(eight), shownAt, T + 24 * HOUR)).toBe(false); + // closing two then opening one is still a net gain of room + const fewer = recordAt([...eight, ...many(2)], T); + expect(finishDue(over([...eight, pull('me', 1)]), fewer, T + 24 * HOUR)).toBe(false); + }); + + it('re-shows after 4 hours when a PR newly crosses the age limit', () => { + const old = pull('me', 20); + const pulls = [pull('me', 1), old]; + const last = recordAt(pulls, T); + expect(finishDue(over(pulls), last, T + 24 * HOUR)).toBe(false); + // same count: the young one aged past 14 days + const aged = { ...pulls[0], ageDays: 15 }; + expect(finishDue(over([aged, old]), last, T + 24 * HOUR)).toBe(true); + }); + + it('ignores a missing or unreadable record (and the old daily key)', () => { + expect(parseFinishRecord(null)).toBeNull(); + expect(parseFinishRecord('2026-10-09')).toBeNull(); + expect(parseFinishRecord('{"at":1}')).toBeNull(); + expect(parseFinishRecord(JSON.stringify(shownAt))).toEqual(shownAt); + }); +}); + +describe('finishStats', () => { + const texts = (c: Parameters[0]) => finishStats(c, LIMITS).map(s => s.text); + + it('shows each number only when it is above zero', () => { + expect(texts({ count: 9, over: ['a', 'b'], ready: 3 })).toEqual([ + '9 open PRs, aim for 7', + '2 PRs older than 14 days', + '3 PRs ready to merge', + ]); + expect(texts({ count: 9, over: [], ready: 0 })).toEqual(['9 open PRs, aim for 7']); + expect(texts({ count: 3, over: ['a'], ready: 1 })).toEqual([ + '1 PR older than 14 days', + '1 PR ready to merge', + ]); + expect(texts({ count: 7, over: [], ready: 0 })).toEqual([]); + }); + + it('points each number at the PRs it counts', () => { + const [, old, ready] = finishStats({ count: 9, over: ['a'], ready: 1 }, LIMITS); + expect(old.counts(pull('me', 15))).toBe(true); + expect(old.counts(pull('me', 14))).toBe(false); + expect(ready.counts(pull('me', 1, { ready: true }))).toBe(true); + expect(ready.counts(pull('me', 1))).toBe(false); + }); +}); diff --git a/frontend-v2/src/model/finishLine.ts b/frontend-v2/src/model/finishLine.ts new file mode 100644 index 00000000..1c3aec17 --- /dev/null +++ b/frontend-v2/src/model/finishLine.ts @@ -0,0 +1,191 @@ +import type { DerivedPull } from '../../../shared/model/status'; +import { isBotLogin } from '../../../shared/model/visibility'; +import type { PullData } from '../../../shared/types'; +import { rowWord } from './actions'; + +/** Over the finish line's limit with more than this many open PRs of your own. */ +export const FINISH_MAX_OPEN = 7; +/** ...or when any of your open PRs is older than this many days. */ +export const FINISH_MAX_DAYS = 14; + +/** localStorage key holding the FinishRecord of the last time it popped up. */ +export const FINISH_LINE_KEY = 'pd2.finishLine'; +/** After it pops up, it stays away at least this long. */ +export const FINISH_COOLDOWN_MS = 4 * 60 * 60 * 1000; + +/** What it showed you last time, so it only comes back when that got worse. */ +export interface FinishRecord { + /** ms when it showed */ + at: number; + /** how many counted open PRs you had then */ + count: number; + /** repo#number of each PR past the age limit then */ + over: string[]; +} + +/** The fields the rule reads off a derived pull. */ +export interface FinishPull { + data: Pick; + ageDays: number; + /** who holds a deploy block on it; a held PR is done, so it doesn't count */ + deployBlockedBy: string[]; + status: DerivedPull['status']; + /** parked (Cryogenic Storage): kept open on purpose, never "ready" */ + cryo: boolean; +} + +/** Can merge now, the quickest way to the finish line. A parked PR can be + * signed off and green, but My work files it under waiting, so this does too. */ +export const isFinishReady = (p: FinishPull) => p.status === 'ready' && !p.cryo; + +export interface FinishLineCase

    { + /** the PRs that put you there, oldest first */ + pulls: P[]; + /** how many of `pulls` can merge now */ + ready: number; + /** how many open PRs counted */ + count: number; + /** repo#number of each counted PR past the age limit */ + over: string[]; + /** how many PRs you still have to merge or close to finish: enough to + * get under the count limit, and every one past the age limit (closing an + * old one counts toward both) */ + toGo: number; +} + +const idOf = (p: FinishPull) => `${p.data.repo}#${p.data.number}`; + +const oldestFirst =

    (a: P, b: P) => b.ageDays - a.ageDays; + +/** + * Your own open, human PRs that aren't held for a deploy, checked against the + * two limits; drafts count only when `countDrafts` is on. Null when you're + * under both. + */ +export function finishLineCase

    ( + pulls: P[], + me: string, + extraBots: ReadonlySet, + limits: { maxOpen: number; maxDays: number; countDrafts?: boolean } +): FinishLineCase

    | null { + const mine = pulls + .filter( + p => + p.data.user.login === me && + p.data.state === 'open' && + (limits.countDrafts || !p.data.draft) && + !p.deployBlockedBy.length && + !isBotLogin(p.data.user.login, extraBots) + ) + .sort(oldestFirst); + const old = mine.filter(p => p.ageDays > limits.maxDays); + const counts = { + count: mine.length, + over: old.map(idOf), + toGo: Math.max(mine.length - limits.maxOpen, old.length), + }; + if (mine.length <= limits.maxOpen && !old.length) return null; + const listed = mine.length > limits.maxOpen ? mine : old; + const ready = listed.filter(isFinishReady).length; + return { + pulls: listed, + ready, + ...counts, + }; +} + +/** + * The listed PRs split the way My work splits them, by the same rowWord, with + * the ready ones lifted out on top: ready to merge, your move, then waiting + * on others. Each group runs oldest first; the row's word says the rest. + */ +export function finishGroups(pulls: DerivedPull[], me: string) { + const worded = pulls + .map(pull => ({ pull, word: rowWord(pull, me) })) + .sort((a, b) => b.pull.ageDays - a.pull.ageDays); + const rest = worded.filter(w => !isFinishReady(w.pull)); + return { + ready: worded.filter(w => isFinishReady(w.pull)), + move: rest.filter(w => w.word.kind === 'do'), + waiting: rest.filter(w => w.word.kind === 'wait'), + }; +} + +/** The record to store when `found` shows at `now`. */ +export const finishRecord = (found: FinishLineCase, now: number): FinishRecord => ({ + at: now, + count: found.count, + over: found.over, +}); + +/** A stored record, or null when it's missing or unreadable. */ +export function parseFinishRecord(raw: string | null): FinishRecord | null { + try { + const r = JSON.parse(raw ?? 'null') as Partial | null; + if (typeof r?.at !== 'number' || typeof r.count !== 'number' || !Array.isArray(r.over)) { + return null; + } + return { at: r.at, count: r.count, over: r.over }; + } catch { + return null; + } +} + +/** + * Whether it should pop up now: you're over a limit, and either it never + * showed, or it's been at least FINISH_COOLDOWN_MS and things got worse since + * (more open PRs, net, or a PR newly past the age limit). + */ +export function finishDue( + found: FinishLineCase | null, + last: FinishRecord | null, + now: number +): boolean { + if (!found) return false; + if (!last) return true; + if (now - last.at < FINISH_COOLDOWN_MS) return false; + return found.count > last.count || found.over.some(id => !last.over.includes(id)); +} + +/** One number at the top of the list you can act on, and which PRs it + * points at, so clicking it can open their group. */ +export interface FinishStat { + key: 'open' | 'old' | 'ready'; + text: string; + /** the PRs this number counts */ + counts: (p: FinishPull) => boolean; +} + +/** "1 PR", "2 PRs" */ +export const prCount = (n: number) => (n === 1 ? '1 PR' : `${n} PRs`); + +/** + * The numbers you can act on, each only when above zero: how many open + * against the aim, how many past the age limit, how many can merge now. + * Waiting on others is left out: there's nothing to do about it here. + */ +export function finishStats( + found: Pick, 'count' | 'over' | 'ready'>, + limits: { maxOpen: number; maxDays: number } +): FinishStat[] { + const stats: FinishStat[] = []; + if (found.count > limits.maxOpen) + stats.push({ + key: 'open', + text: `${found.count} open PRs, aim for ${limits.maxOpen}`, + counts: () => true, + }); + if (found.over.length) + stats.push({ + key: 'old', + text: `${prCount(found.over.length)} older than ${limits.maxDays} days`, + counts: p => p.ageDays > limits.maxDays, + }); + if (found.ready) + stats.push({ + key: 'ready', + text: `${prCount(found.ready)} ready to merge`, + counts: isFinishReady, + }); + return stats; +} diff --git a/frontend-v2/src/model/jail.test.ts b/frontend-v2/src/model/jail.test.ts deleted file mode 100644 index 0069a243..00000000 --- a/frontend-v2/src/model/jail.test.ts +++ /dev/null @@ -1,200 +0,0 @@ -import { describe, expect, it } from 'vitest'; -import { - JAIL_COOLDOWN_MS, - jailCase, - jailDue, - jailRecord, - parseJailRecord, - type JailPull, -} from './jail'; - -const LIMITS = { maxOpen: 7, maxDays: 14 }; -const NO_BOTS: ReadonlySet = new Set(); - -let n = 0; -function pull( - login: string, - ageDays: number, - o: { draft?: boolean; deployBlockedBy?: string[]; ready?: boolean; cryo?: boolean } = {} -): JailPull { - n += 1; - return { - data: { - repo: 'iFixit/ifixit', - number: n, - title: `PR ${n}`, - draft: o.draft ?? false, - state: 'open', - user: { login }, - }, - ageDays, - deployBlockedBy: o.deployBlockedBy ?? [], - status: o.ready ? 'ready' : 'needs_cr', - cryo: o.cryo ?? false, - }; -} - -const many = (count: number, login = 'me', ageDays = 1) => - Array.from({ length: count }, () => pull(login, ageDays)); - -describe('jailCase', () => { - // [open, ready] -> the why line; limit 7 - it.each([ - [20, 0, '20 open PRs. Parole at 7. Close 13 to get out.'], - [20, 3, '20 open PRs. Parole at 7. Merge your 3 ready ones and close 10 more.'], - [10, 3, '10 open PRs. Parole at 7. Merge your 3 ready ones and you’re out.'], - [12, 6, '12 open PRs. Parole at 7. Merge 5 of your 6 ready ones and you’re out.'], - [8, 1, '8 open PRs. Parole at 7. Merge your ready one and you’re out.'], - [9, 1, '9 open PRs. Parole at 7. Merge your ready one and close 1 more.'], - ])('with %i open and %i ready, points at the ready ones first', (open, ready, why) => { - const pulls = [ - ...many(open - ready), - ...Array.from({ length: ready }, () => pull('me', 1, { ready: true })), - ]; - expect(jailCase(pulls, 'me', NO_BOTS, LIMITS)?.why).toBe(why); - }); - - it('never counts a parked PR as ready', () => { - const parked = pull('me', 1, { ready: true, cryo: true }); - expect(jailCase([...many(7), parked], 'me', NO_BOTS, LIMITS)?.why).toBe( - '8 open PRs. Parole at 7. Close 1 to get out.' - ); - }); - - it('won’t promise you’re out while an old PR would still hold you', () => { - const old = pull('me', 40); - const pulls = [...many(6), old, pull('me', 1, { ready: true })]; - expect(jailCase(pulls, 'me', NO_BOTS, LIMITS)?.why).toBe( - `8 open PRs. Parole at 7. Merge your ready one to get under 7. #${old.data.number} is still past 14 days.` - ); - // an old PR that's ready leaves with the merge, so you are out - const oldReady = [...many(7), pull('me', 40, { ready: true })]; - expect(jailCase(oldReady, 'me', NO_BOTS, LIMITS)?.why).toBe( - '8 open PRs. Parole at 7. Merge your ready one and you’re out.' - ); - }); - - it('counts what’s left to close, the old ones included', () => { - // 9 open (2 over the limit) and 3 old: closing the 3 old ones frees you - const pulls = [...many(6), pull('me', 20), pull('me', 30), pull('me', 40)]; - expect(jailCase(pulls, 'me', NO_BOTS, LIMITS)?.toGo).toBe(3); - // 12 open (5 over) and 1 old: 5 to go, the old one among them - expect(jailCase([...many(11), pull('me', 20)], 'me', NO_BOTS, LIMITS)?.toGo).toBe(5); - }); - - it('says merge when the one old PR is ready, and counts several', () => { - const old = pull('me', 30, { ready: true }); - expect(jailCase([old], 'me', NO_BOTS, LIMITS)?.why).toBe( - `#${old.data.number} has been open 30 days and it’s ready. Merge it to get out.` - ); - expect(jailCase([pull('me', 20), pull('me', 15)], 'me', NO_BOTS, LIMITS)?.why).toBe( - '2 PRs open past 14 days. Merge or close them to get out.' - ); - }); - - it('stays free at the open-PR limit', () => { - expect(jailCase(many(7), 'me', NO_BOTS, LIMITS)).toBeNull(); - }); - - it('jails past the open-PR limit, listing them oldest first', () => { - const pulls = [...many(7), pull('me', 9)]; - const c = jailCase(pulls, 'me', NO_BOTS, LIMITS); - expect(c?.pulls).toHaveLength(8); - expect(c?.pulls[0].ageDays).toBe(9); - expect(c?.why).toBe('8 open PRs. Parole at 7. Close 1 to get out.'); - expect(c?.count).toBe(8); - expect(c?.toGo).toBe(1); - }); - - it('jails one PR past the age limit, and lists only the old ones', () => { - const old = pull('me', 21); - const c = jailCase([pull('me', 2), old, pull('me', 14)], 'me', NO_BOTS, LIMITS); - expect(c?.pulls).toEqual([old]); - expect(c?.why).toBe( - `#${old.data.number} has been open 21 days. Parole at 14 days. Merge or close it to get out.` - ); - expect(c?.count).toBe(3); - expect(c?.over).toEqual([`iFixit/ifixit#${old.data.number}`]); - expect(c?.toGo).toBe(1); - }); - - it('ignores other people’s PRs', () => { - expect( - jailCase([...many(9, 'someone'), pull('someone', 90)], 'me', NO_BOTS, LIMITS) - ).toBeNull(); - }); - - it('ignores drafts unless they count', () => { - const drafts = Array.from({ length: 9 }, () => pull('me', 90, { draft: true })); - expect(jailCase(drafts, 'me', NO_BOTS, LIMITS)).toBeNull(); - const counted = jailCase(drafts, 'me', NO_BOTS, { ...LIMITS, countDrafts: true }); - expect(counted?.pulls).toHaveLength(9); - }); - - it('ignores bots, by suffix and by the config list', () => { - expect(jailCase(many(9, 'dependabot[bot]'), 'dependabot[bot]', NO_BOTS, LIMITS)).toBeNull(); - expect(jailCase(many(9, 'renovate'), 'renovate', new Set(['renovate']), LIMITS)).toBeNull(); - }); - - it('ignores PRs held for a deploy', () => { - const held = Array.from({ length: 9 }, () => pull('me', 90, { deployBlockedBy: ['qa'] })); - expect(jailCase(held, 'me', NO_BOTS, LIMITS)).toBeNull(); - }); -}); - -describe('jailDue', () => { - const HOUR = 60 * 60 * 1000; - const T = 1_000_000_000_000; - const over = (pulls: JailPull[]) => jailCase(pulls, 'me', NO_BOTS, LIMITS); - const recordAt = (pulls: JailPull[], at: number) => { - const c = over(pulls); - if (!c) throw new Error('expected to be over a limit'); - return jailRecord(c, at); - }; - const eight = many(8); - const shownAt = recordAt(eight, T); - - it('never shows while you are under the limits', () => { - expect(jailDue(over(many(7)), null, T)).toBe(false); - }); - - it('shows the first time', () => { - expect(jailDue(over(eight), null, T)).toBe(true); - }); - - it('does not re-show on a refresh', () => { - expect(jailDue(over(eight), shownAt, T + 1000)).toBe(false); - }); - - it('does not re-show within 4 hours, even when it got worse', () => { - expect(jailDue(over([...eight, pull('me', 1)]), shownAt, T + 3 * HOUR)).toBe(false); - }); - - it('re-shows after 4 hours when you have more open PRs', () => { - expect(jailDue(over([...eight, pull('me', 1)]), shownAt, T + JAIL_COOLDOWN_MS)).toBe(true); - }); - - it('does not re-show after 4 hours when it is the same or better', () => { - expect(jailDue(over(eight), shownAt, T + 24 * HOUR)).toBe(false); - // closing two then opening one is still a net gain of room - const fewer = recordAt([...eight, ...many(2)], T); - expect(jailDue(over([...eight, pull('me', 1)]), fewer, T + 24 * HOUR)).toBe(false); - }); - - it('re-shows after 4 hours when a PR newly crosses the age limit', () => { - const old = pull('me', 20); - const pulls = [pull('me', 1), old]; - const last = recordAt(pulls, T); - expect(jailDue(over(pulls), last, T + 24 * HOUR)).toBe(false); - // same count: the young one aged past 14 days - const aged = { ...pulls[0], ageDays: 15 }; - expect(jailDue(over([aged, old]), last, T + 24 * HOUR)).toBe(true); - }); - - it('ignores a missing or unreadable record (and the old daily key)', () => { - expect(parseJailRecord(null)).toBeNull(); - expect(parseJailRecord('2026-10-09')).toBeNull(); - expect(parseJailRecord('{"at":1}')).toBeNull(); - expect(parseJailRecord(JSON.stringify(shownAt))).toEqual(shownAt); - }); -}); diff --git a/frontend-v2/src/model/jail.ts b/frontend-v2/src/model/jail.ts deleted file mode 100644 index 65f5504b..00000000 --- a/frontend-v2/src/model/jail.ts +++ /dev/null @@ -1,194 +0,0 @@ -import type { DerivedPull } from '../../../shared/model/status'; -import { isBotLogin } from '../../../shared/model/visibility'; -import type { PullData } from '../../../shared/types'; -import { rowWord } from './actions'; - -/** Jail when you have more than this many open PRs of your own. */ -export const JAIL_MAX_OPEN = 7; -/** Jail when any of your open PRs is older than this many days. */ -export const JAIL_MAX_DAYS = 14; - -/** localStorage key holding the JailRecord of the last time jail showed. */ -export const JAIL_KEY = 'pd2.jail'; -/** After jail shows, it stays away at least this long. */ -export const JAIL_COOLDOWN_MS = 4 * 60 * 60 * 1000; - -/** What jail showed you last time, so it only comes back when that got worse. */ -export interface JailRecord { - /** ms when it showed */ - at: number; - /** how many counted open PRs you had then */ - count: number; - /** repo#number of each PR past the age limit then */ - over: string[]; -} - -/** The fields the rule reads off a derived pull. */ -export interface JailPull { - data: Pick; - ageDays: number; - /** who holds a deploy block on it; a held PR is done, so it doesn't count */ - deployBlockedBy: string[]; - status: DerivedPull['status']; - /** parked (Cryogenic Storage): kept open on purpose, never "ready" */ - cryo: boolean; -} - -/** Can merge now, the quickest way out. A parked PR can be signed off and - * green, but My work files it under waiting, so jail does too. */ -export const isJailReady = (p: JailPull) => p.status === 'ready' && !p.cryo; - -export interface JailCase

    { - /** the PRs that put you there, oldest first */ - pulls: P[]; - /** one line saying why, and how to get out */ - why: string; - /** how many open PRs counted */ - count: number; - /** repo#number of each counted PR past the age limit */ - over: string[]; - /** how many PRs you still have to merge or close to be free: enough to - * get under the count limit, and every one past the age limit (closing an - * old one counts toward both) */ - toGo: number; -} - -const idOf = (p: JailPull) => `${p.data.repo}#${p.data.number}`; - -const oldestFirst =

    (a: P, b: P) => b.ageDays - a.ageDays; - -/** - * Your own open, human PRs that aren't held for a deploy, checked against the - * two limits; drafts count only when `countDrafts` is on. Null when you're - * under both. - */ -export function jailCase

    ( - pulls: P[], - me: string, - extraBots: ReadonlySet, - limits: { maxOpen: number; maxDays: number; countDrafts?: boolean } -): JailCase

    | null { - const mine = pulls - .filter( - p => - p.data.user.login === me && - p.data.state === 'open' && - (limits.countDrafts || !p.data.draft) && - !p.deployBlockedBy.length && - !isBotLogin(p.data.user.login, extraBots) - ) - .sort(oldestFirst); - const old = mine.filter(p => p.ageDays > limits.maxDays); - const counts = { - count: mine.length, - over: old.map(idOf), - toGo: Math.max(mine.length - limits.maxOpen, old.length), - }; - if (mine.length > limits.maxOpen) { - const why = countWhy( - mine.length, - limits.maxOpen, - mine.filter(isJailReady).length, - old.filter(p => !isJailReady(p)), - limits.maxDays - ); - return { pulls: mine, why, ...counts }; - } - if (old.length) { - return { pulls: old, why: ageWhy(old, limits.maxDays), ...counts }; - } - return null; -} - -/** The count line, pointing at the ready PRs first since merging one is - * the quickest way to lower the count. `stillOld` are the PRs past the age - * limit that merging the ready ones won't clear, so it never promises - * "you're out" while one is left. */ -// ponytail: when more are ready than needed, an old ready PR left unmerged -// still holds you; the line doesn't say which ready ones to pick -function countWhy( - n: number, - max: number, - ready: number, - stillOld: JailPull[], - maxDays: number -): string { - const need = n - max; - const head = `${n} open PRs. Parole at ${max}.`; - const yours = ready === 1 ? 'your ready one' : `your ${ready} ready ones`; - const out = stillOld.length ? `to get under ${max}` : 'and you’re out'; - const tail = !stillOld.length - ? '' - : stillOld.length === 1 - ? ` #${stillOld[0].data.number} is still past ${maxDays} days.` - : ` ${stillOld.length} PRs are still past ${maxDays} days.`; - if (!ready) - return `${head} Close ${need} to get ${stillOld.length ? `under ${max}` : 'out'}.${tail}`; - if (ready < need) return `${head} Merge ${yours} and close ${need - ready} more.${tail}`; - if (ready === need) return `${head} Merge ${yours} ${out}.${tail}`; - return `${head} Merge ${need} of ${yours} ${out}.${tail}`; -} - -function ageWhy(old: JailPull[], maxDays: number): string { - if (old.length > 1) { - return `${old.length} PRs open past ${maxDays} days. Merge or close them to get out.`; - } - const [p] = old; - const head = `#${p.data.number} has been open ${p.ageDays} days`; - return isJailReady(p) - ? `${head} and it’s ready. Merge it to get out.` - : `${head}. Parole at ${maxDays} days. Merge or close it to get out.`; -} - -/** - * A jail's PRs split the way My work splits them, by the same rowWord, with - * the ready ones lifted out on top: ready to merge, your move, then waiting - * on others. Each group runs oldest first; the row's word says the rest. - */ -export function jailGroups(pulls: DerivedPull[], me: string) { - const worded = pulls - .map(pull => ({ pull, word: rowWord(pull, me) })) - .sort((a, b) => b.pull.ageDays - a.pull.ageDays); - const rest = worded.filter(w => !isJailReady(w.pull)); - return { - ready: worded.filter(w => isJailReady(w.pull)), - move: rest.filter(w => w.word.kind === 'do'), - waiting: rest.filter(w => w.word.kind === 'wait'), - }; -} - -/** The record to store when `jail` shows at `now`. */ -export const jailRecord = (jail: JailCase, now: number): JailRecord => ({ - at: now, - count: jail.count, - over: jail.over, -}); - -/** A stored record, or null when it's missing or unreadable. */ -export function parseJailRecord(raw: string | null): JailRecord | null { - try { - const r = JSON.parse(raw ?? 'null') as Partial | null; - if (typeof r?.at !== 'number' || typeof r.count !== 'number' || !Array.isArray(r.over)) { - return null; - } - return { at: r.at, count: r.count, over: r.over }; - } catch { - return null; - } -} - -/** - * Whether jail should drop now: you're over a limit, and either it never - * showed, or it's been at least JAIL_COOLDOWN_MS and things got worse since - * (more open PRs, net, or a PR newly past the age limit). - */ -export function jailDue( - jail: JailCase | null, - last: JailRecord | null, - now: number -): boolean { - if (!jail) return false; - if (!last) return true; - if (now - last.at < JAIL_COOLDOWN_MS) return false; - return jail.count > last.count || jail.over.some(id => !last.over.includes(id)); -} diff --git a/frontend-v2/src/settings.test.ts b/frontend-v2/src/settings.test.ts new file mode 100644 index 00000000..5f865ff5 --- /dev/null +++ b/frontend-v2/src/settings.test.ts @@ -0,0 +1,9 @@ +import { describe, expect, it } from 'vitest'; +import { DEFAULT_SETTINGS } from './settings'; + +describe('Finish line is opt-in', () => { + it('shows only the header count by default, and closes without a hold', () => { + expect(DEFAULT_SETTINGS.finishLineMode).toBe('manual'); + expect(DEFAULT_SETTINGS.finishLineHoldToClose).toBe(false); + }); +}); diff --git a/frontend-v2/src/settings.ts b/frontend-v2/src/settings.ts index 0c6feae6..a3fc0a48 100644 --- a/frontend-v2/src/settings.ts +++ b/frontend-v2/src/settings.ts @@ -1,5 +1,5 @@ import { STARVE_DAYS } from '../../shared/model/status'; -import { JAIL_MAX_DAYS, JAIL_MAX_OPEN } from './model/jail'; +import { FINISH_MAX_DAYS, FINISH_MAX_OPEN } from './model/finishLine'; import { createPersistentStore } from './storage'; /** one named review circle; DEFAULT_TEAM_NAME is what quick add-to-team @@ -111,15 +111,19 @@ export interface Settings { * or age tooltip) before it opens, so brushing the pointer across the board * doesn't flash panels open. 0 = open instantly; a click always bypasses it. */ hoverDelayMs: number; - /** PR jail: 'auto' pops it up when you're over the limits, 'manual' only - * shows the header lock (click to open), 'off' shows neither */ - jailMode: 'auto' | 'manual' | 'off'; - /** PR jail pops up when you have more than this many open PRs of your own */ - jailMaxOpen: number; + /** Finish line: 'auto' pops it up + * when you're over the limits, 'manual' only shows the header count + * (click to open), 'off' shows neither. Opt-in: 'manual' by default. */ + finishLineMode: 'auto' | 'manual' | 'off'; + /** Finish line counts you over when you have more than this many open PRs */ + finishLineMaxOpen: number; /** ...or when any of them has been open longer than this many days */ - jailMaxDays: number; - /** whether your draft PRs count toward PR jail */ - jailCountDrafts: boolean; + finishLineMaxDays: number; + /** whether your draft PRs count toward Finish line */ + finishLineCountDrafts: boolean; + /** Finish line closes only by holding a button down; Esc and the close + * button do nothing. Off by default, for people who want the friction. */ + finishLineHoldToClose: boolean; } /** The heaviest age text tier (red), derived from ageWarnDays rather than @@ -172,21 +176,14 @@ export const DEFAULT_SETTINGS: Settings = { repoQueueCap: 15, openPrsNewTab: true, hoverDelayMs: 250, - jailMode: 'auto', - jailMaxOpen: JAIL_MAX_OPEN, - jailMaxDays: JAIL_MAX_DAYS, - jailCountDrafts: false, + finishLineMode: 'manual', + finishLineMaxOpen: FINISH_MAX_OPEN, + finishLineMaxDays: FINISH_MAX_DAYS, + finishLineCountDrafts: false, + finishLineHoldToClose: false, }; const store = createPersistentStore('pd2.settings', DEFAULT_SETTINGS); -// jailOn (an on/off switch, live for a day) became jailMode; an Off carries over -{ - const saved = store.get() as Settings & { jailOn?: boolean }; - if (saved.jailOn !== undefined) { - const { jailOn, ...rest } = saved; - store.set({ ...rest, jailMode: jailOn ? rest.jailMode : 'off' }); - } -} /** Union of every personal roster, sorted — "your people": the set that * leads the review queues and wears the teammate corner heart. */ diff --git a/frontend-v2/src/styles.css b/frontend-v2/src/styles.css index ff41a068..a1d15432 100644 --- a/frontend-v2/src/styles.css +++ b/frontend-v2/src/styles.css @@ -1007,10 +1007,10 @@ input[type="checkbox"] { font-weight: 600; } -/* PR jail's snooze key (components/JailMode.tsx): a slow wiggle in the - keyhole while it waits to be held. Transform only, and still when motion - is reduced. */ -@keyframes jail-key-wiggle { +/* Finish line's hold button (components/FinishLine.tsx): the flag gives a + slow wiggle while it waits to be held. Transform only, and still when + motion is reduced. */ +@keyframes finish-flag-wiggle { 0%, 60%, 100% { @@ -1026,31 +1026,12 @@ input[type="checkbox"] { rotate: -6deg; } } -.jail-key-wiggle { - animation: jail-key-wiggle 2.8s ease-in-out infinite; -} -/* ...and the shackle opening once it has turned: up out of the body, then - over its right leg (x 30) to the other side, like a padlock swinging - open. Reduced motion lands on the open lock with no swing. */ -@keyframes jail-shackle-open { - from { - transform: translateY(0) scaleX(1); - } - 35% { - transform: translateY(-5px) scaleX(1); - } - to { - transform: translateY(-5px) scaleX(-1); - } -} -.jail-shackle-open { - transform-origin: 30px 18px; - transform: translateY(-5px) scaleX(-1); - animation: jail-shackle-open 600ms cubic-bezier(0.22, 1, 0.36, 1); +.finish-flag-wiggle { + animation: finish-flag-wiggle 2.8s ease-in-out infinite; } /* ...and a merged or closed PR's row sliding out of the list while the countdown ticks. */ -@keyframes jail-row-leave { +@keyframes finish-row-leave { to { opacity: 0; transform: translateX(1.5rem); @@ -1058,17 +1039,16 @@ input[type="checkbox"] { padding-block: 0; } } -.jail-row-leave { +.finish-row-leave { max-height: 8rem; overflow: hidden; - animation: jail-row-leave 450ms cubic-bezier(0.5, 0, 0.75, 0) forwards; + animation: finish-row-leave 450ms cubic-bezier(0.5, 0, 0.75, 0) forwards; } @media (prefers-reduced-motion: reduce) { - .jail-key-wiggle, - .jail-shackle-open { + .finish-flag-wiggle { animation: none; } - .jail-row-leave { + .finish-row-leave { animation-duration: 1ms; } }