diff --git a/CLAUDE.md b/CLAUDE.md index c13a0b1e0..8c011c0a0 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -25,6 +25,12 @@ bun run site:build # Astro marketing site build **npm CLI is also required** (alongside bun) for one specific case: `scripts/build-grammars.sh` and `scripts/build-sql-grammar.sh` use `npm pack` to fetch grammar tarballs from the npm registry. Bun has no equivalent registry-fetch command yet (`bun pm pack` only packs the local package). +## Fresh worktrees: CRLF checkout vs biome's LF requirement + +The repo has `core.autocrlf=true` set locally, so a **fresh `git worktree add` checks every file out with CRLF line endings** — while biome (which the lefthook pre-commit runs on staged files) requires LF. The main checkout's files are LF on disk only because they predate the autocrlf setting; a new worktree is CRLF everywhere, so the very first commit touching any file fails the format check with whole-file `␍`-removal diffs. Fix: after editing in a fresh worktree, run `bunx biome check --write ` before committing (committed blobs are LF either way — only the working-tree bytes are wrong). Don't mass-format untouched files: that surfaces unrelated pre-existing lint fixes as spurious diffs. + +Two other fresh-worktree facts: run `bun install` at the worktree root before anything else (node_modules is per-worktree), and Playwright e2e (`bun run test:e2e`) reuses any server already listening on :1420 (`reuseExistingServer`) — if the main checkout's dev server is running, e2e silently tests the *main checkout's* code, not the worktree's. Stop it first or the results are meaningless. + ## React Compiler escape hatch The React Compiler (`babel-plugin-react-compiler`, wired into `@vitejs/plugin-react` in `vite.config.ts`) auto-memoizes function components and hooks at build time. If a specific component breaks under compilation — usually because it relied on referential identity for a side effect — opt it out file-locally with the `"use no memo"` directive at the very top of the file: diff --git a/src/BorgDock.Tauri/.storybook/main.ts b/src/BorgDock.Tauri/.storybook/main.ts index 54b891c8a..ea99923d7 100644 --- a/src/BorgDock.Tauri/.storybook/main.ts +++ b/src/BorgDock.Tauri/.storybook/main.ts @@ -1,13 +1,25 @@ // .storybook/main.ts -import type { StorybookConfig } from '@storybook/react-vite'; +import { createRequire } from 'node:module'; +import { dirname, join, resolve } from 'node:path'; import { fileURLToPath } from 'node:url'; -import { dirname, resolve } from 'node:path'; +import type { StorybookConfig } from '@storybook/react-vite'; import tailwindcss from '@tailwindcss/vite'; import { viteStaticCopy } from 'vite-plugin-static-copy'; const here = dirname(fileURLToPath(import.meta.url)); +const require = createRequire(import.meta.url); +// Resolve web-tree-sitter from wherever the package manager actually +// installed it (npm: nested in src/BorgDock.Tauri/node_modules, bun +// workspace: hoisted to repo root) — same trick as vite.config.ts. +// Forward slashes only: vite-plugin-static-copy hands `src` to fast-glob, +// which treats Windows backslashes as escape characters. +const webTreeSitterWasm = join( + dirname(require.resolve('web-tree-sitter')), + 'web-tree-sitter.wasm', +).replace(/\\/g, '/'); + const config: StorybookConfig = { framework: { name: '@storybook/react-vite', @@ -31,7 +43,7 @@ const config: StorybookConfig = { viteStaticCopy({ targets: [ { - src: 'node_modules/web-tree-sitter/web-tree-sitter.wasm', + src: webTreeSitterWasm, dest: '.', rename: { stripBase: true }, }, @@ -67,25 +79,71 @@ const config: StorybookConfig = { { find: '@tauri-apps/api/core', replacement: resolve(here, 'mocks/tauri-core.ts') }, { find: '@tauri-apps/api/event', replacement: resolve(here, 'mocks/tauri-event.ts') }, { find: '@tauri-apps/api/window', replacement: resolve(here, 'mocks/tauri-api-window.ts') }, - { find: '@tauri-apps/api/webviewWindow', replacement: resolve(here, 'mocks/tauri-api-webviewWindow.ts') }, + { + find: '@tauri-apps/api/webviewWindow', + replacement: resolve(here, 'mocks/tauri-api-webviewWindow.ts'), + }, { find: '@tauri-apps/api/app', replacement: resolve(here, 'mocks/tauri-api-app.ts') }, { find: '@tauri-apps/api/dpi', replacement: resolve(here, 'mocks/tauri-api-dpi.ts') }, - { find: '@tauri-apps/plugin-opener', replacement: resolve(here, 'mocks/tauri-plugin-opener.ts') }, - { find: '@tauri-apps/plugin-store', replacement: resolve(here, 'mocks/tauri-plugin-store.ts') }, - { find: '@tauri-apps/plugin-clipboard-manager', replacement: resolve(here, 'mocks/tauri-plugin-clipboard-manager.ts') }, - { find: '@tauri-apps/plugin-dialog', replacement: resolve(here, 'mocks/tauri-plugin-dialog.ts') }, + { + find: '@tauri-apps/plugin-opener', + replacement: resolve(here, 'mocks/tauri-plugin-opener.ts'), + }, + { + find: '@tauri-apps/plugin-store', + replacement: resolve(here, 'mocks/tauri-plugin-store.ts'), + }, + { + find: '@tauri-apps/plugin-clipboard-manager', + replacement: resolve(here, 'mocks/tauri-plugin-clipboard-manager.ts'), + }, + { + find: '@tauri-apps/plugin-dialog', + replacement: resolve(here, 'mocks/tauri-plugin-dialog.ts'), + }, { find: '@tauri-apps/plugin-fs', replacement: resolve(here, 'mocks/tauri-plugin-fs.ts') }, - { find: '@tauri-apps/plugin-autostart', replacement: resolve(here, 'mocks/tauri-plugin-autostart.ts') }, + { + find: '@tauri-apps/plugin-autostart', + replacement: resolve(here, 'mocks/tauri-plugin-autostart.ts'), + }, { find: '@/services/windows', replacement: resolve(here, 'mocks/services-windows.ts') }, - { find: '@/services/ado/workitems', replacement: resolve(here, 'mocks/services-ado-workitems.ts') }, - { find: '@/services/github/pulls', replacement: resolve(here, 'mocks/services-github-pulls.ts') }, - { find: '@/services/github/checks', replacement: resolve(here, 'mocks/services-github-checks.ts') }, - { find: '@/services/github/auth', replacement: resolve(here, 'mocks/services-github-auth.ts') }, - { find: '@/services/github/reviewThreads', replacement: resolve(here, 'mocks/services-github-reviewThreads.ts') }, - { find: '@/services/github/singleton', replacement: resolve(here, 'mocks/services-github-singleton.ts') }, - { find: '@/services/github/mutations', replacement: resolve(here, 'mocks/services-github-mutations.ts') }, + { + find: '@/services/ado/workitems', + replacement: resolve(here, 'mocks/services-ado-workitems.ts'), + }, + { + find: '@/services/github/pulls', + replacement: resolve(here, 'mocks/services-github-pulls.ts'), + }, + { + find: '@/services/github/checks', + replacement: resolve(here, 'mocks/services-github-checks.ts'), + }, + { + find: '@/services/github/polling', + replacement: resolve(here, 'mocks/services-github-polling.ts'), + }, + { + find: '@/services/github/auth', + replacement: resolve(here, 'mocks/services-github-auth.ts'), + }, + { + find: '@/services/github/reviewThreads', + replacement: resolve(here, 'mocks/services-github-reviewThreads.ts'), + }, + { + find: '@/services/github/singleton', + replacement: resolve(here, 'mocks/services-github-singleton.ts'), + }, + { + find: '@/services/github/mutations', + replacement: resolve(here, 'mocks/services-github-mutations.ts'), + }, // Anchored regex: matches ONLY the exact bare barrel import, not sub-modules. - { find: /^@\/services\/github$/, replacement: resolve(here, 'mocks/services-github-barrel.ts') }, + { + find: /^@\/services\/github$/, + replacement: resolve(here, 'mocks/services-github-barrel.ts'), + }, { find: '@/services/pr-actions', replacement: resolve(here, 'mocks/services-pr-actions.ts') }, { find: '@/generated/changelog', replacement: resolve(here, 'mocks/generated-changelog.ts') }, // Bare '@' catch-all goes last so specific mocks above take priority. diff --git a/src/BorgDock.Tauri/.storybook/mocks/control.ts b/src/BorgDock.Tauri/.storybook/mocks/control.ts index f18b3d40b..1e4e75e30 100644 --- a/src/BorgDock.Tauri/.storybook/mocks/control.ts +++ b/src/BorgDock.Tauri/.storybook/mocks/control.ts @@ -3,12 +3,17 @@ // Singleton control surface used by the Tauri mocks and by story decorators. // Lives on window so dynamic-imported mocks and the React tree can both reach it. -import type { Release } from '../../src/types/whats-new'; -import type { WorkItem, WorkItemComment } from '../../src/types/work-item'; -import type { PullRequest, PullRequestCommit, PullRequestFileChange } from '../../src/types/pull-request'; import type { CheckRun } from '../../src/types/check-run'; -import type { ReviewThread } from '../../src/types/review-thread'; import type { ClaudeReviewComment } from '../../src/types/claude-review'; +import type { + PullRequest, + PullRequestCommit, + PullRequestFileChange, + PullRequestWithChecks, +} from '../../src/types/pull-request'; +import type { ReviewThread } from '../../src/types/review-thread'; +import type { Release } from '../../src/types/whats-new'; +import type { WorkItem, WorkItemComment } from '../../src/types/work-item'; export interface InvokeRecord { command: string; @@ -93,23 +98,41 @@ export interface RawReview { export type GithubResponses = { getOpenPRs?: | PullRequest[] - | ((args: { owner: string; repo: string }) => PullRequest[] | Promise | Promise); + | ((args: { + owner: string; + repo: string; + }) => PullRequest[] | Promise | Promise); getCheckRunsForRef?: | CheckRun[] | ((args: { ref: string }) => CheckRun[] | Promise | Promise); + pollOpenPrsAggregate?: + | PullRequestWithChecks[] + | ((args: { + owner: string; + repo: string; + }) => PullRequestWithChecks[] | Promise | Promise); tokenGetter?: () => string | Promise; // Phase 11 / FilesTab additions - getPRFiles?: PullRequestFileChange[] | (() => PullRequestFileChange[] | Promise); - getCommitFiles?: PullRequestFileChange[] | (() => PullRequestFileChange[] | Promise); + getPRFiles?: + | PullRequestFileChange[] + | (() => PullRequestFileChange[] | Promise); + getCommitFiles?: + | PullRequestFileChange[] + | (() => PullRequestFileChange[] | Promise); getPRCommits?: PullRequestCommit[] | (() => PullRequestCommit[] | Promise); getReviewThreads?: ReviewThread[] | (() => ReviewThread[] | Promise); // DiscussionTab additions getReviews?: RawReview[] | (() => RawReview[] | Promise); - getAllComments?: ClaudeReviewComment[] | (() => ClaudeReviewComment[] | Promise); + getAllComments?: + | ClaudeReviewComment[] + | (() => ClaudeReviewComment[] | Promise); }; // Phase 11 — pr-actions overrides -export type PrActionResponses = Record unknown)>; +export type PrActionResponses = Record< + string, + '__throw__' | '__fail__' | ((args: unknown) => unknown) +>; export interface StorybookTauriControl { channels: Map>; diff --git a/src/BorgDock.Tauri/.storybook/mocks/services-github-barrel.ts b/src/BorgDock.Tauri/.storybook/mocks/services-github-barrel.ts index 89e906722..6a44fad05 100644 --- a/src/BorgDock.Tauri/.storybook/mocks/services-github-barrel.ts +++ b/src/BorgDock.Tauri/.storybook/mocks/services-github-barrel.ts @@ -9,9 +9,10 @@ // the production index.ts, which re-exports via relative paths — and // those relative paths bypass our sub-path aliases. -export * from './services-github-pulls'; -export * from './services-github-checks'; export * from './services-github-auth'; -export * from './services-github-reviewThreads'; +export * from './services-github-checks'; +export * from './services-github-polling'; +export * from './services-github-pulls'; export * from './services-github-reviews'; +export * from './services-github-reviewThreads'; export * from './services-github-singleton'; diff --git a/src/BorgDock.Tauri/.storybook/mocks/services-github-polling.ts b/src/BorgDock.Tauri/.storybook/mocks/services-github-polling.ts new file mode 100644 index 000000000..178c60c73 --- /dev/null +++ b/src/BorgDock.Tauri/.storybook/mocks/services-github-polling.ts @@ -0,0 +1,22 @@ +// .storybook/mocks/services-github-polling.ts +// +// Drop-in replacement for @/services/github/polling. Stories that exercise +// the polling cycle stub responses via: +// getControl().githubResponses.pollOpenPrsAggregate = [prWithChecksFixture]; +// or a function for pending/rejected promises: +// getControl().githubResponses.pollOpenPrsAggregate = () => new Promise(() => {}); + +import type { PullRequestWithChecks } from '../../src/types'; +import { getControl } from './control'; + +export const POLL_OPEN_PRS_QUERY = '<>'; + +export async function pollOpenPrsAggregate( + _client: unknown, + owner: string, + repo: string, +): Promise { + const r = getControl().githubResponses.pollOpenPrsAggregate; + if (typeof r === 'function') return r({ owner, repo }); + return r ?? []; +} diff --git a/src/BorgDock.Tauri/src/components/__tests__/window-title-bar-contract.test.tsx b/src/BorgDock.Tauri/src/components/__tests__/window-title-bar-contract.test.tsx index 474ef3f01..ea6b1f4cb 100644 --- a/src/BorgDock.Tauri/src/components/__tests__/window-title-bar-contract.test.tsx +++ b/src/BorgDock.Tauri/src/components/__tests__/window-title-bar-contract.test.tsx @@ -133,12 +133,13 @@ function makePr(): PullRequestWithChecks { commitCount: 1, requestedReviewers: [], }, - checks: [], overallStatus: 'green', failedCheckNames: [], + failedCheckSuiteIds: [], pendingCheckNames: [], passedCount: 0, skippedCount: 0, + totalCheckCount: 0, }; } @@ -301,7 +302,10 @@ describe('Pop-out window title bars — drag region clickability', () => { it('PrDetailPanel: WindowControls cluster carries bd-wc-group (no-drag escape hatch)', () => { const { container } = render(); const cluster = container.querySelector('.bd-wc-group'); - expect(cluster, 'PrDetailPanel: cluster missing — buttons would not be clickable').not.toBeNull(); + expect( + cluster, + 'PrDetailPanel: cluster missing — buttons would not be clickable', + ).not.toBeNull(); }); it('WhatsNewApp: WindowControls cluster carries bd-wc-group', () => { diff --git a/src/BorgDock.Tauri/src/components/focus/__tests__/helpers.ts b/src/BorgDock.Tauri/src/components/focus/__tests__/helpers.ts index 8556b27cf..cbf38cfbc 100644 --- a/src/BorgDock.Tauri/src/components/focus/__tests__/helpers.ts +++ b/src/BorgDock.Tauri/src/components/focus/__tests__/helpers.ts @@ -34,12 +34,13 @@ export function makePr( requestedReviewers: [], ...overrides, }, - checks: [], overallStatus: 'green', failedCheckNames: [], + failedCheckSuiteIds: [], pendingCheckNames: [], passedCount: 1, skippedCount: 0, + totalCheckCount: 0, }; } diff --git a/src/BorgDock.Tauri/src/components/pr-detail/ChecksTab.stories.tsx b/src/BorgDock.Tauri/src/components/pr-detail/ChecksTab.stories.tsx index a1faa6f86..8557e1162 100644 --- a/src/BorgDock.Tauri/src/components/pr-detail/ChecksTab.stories.tsx +++ b/src/BorgDock.Tauri/src/components/pr-detail/ChecksTab.stories.tsx @@ -2,17 +2,19 @@ import type { Meta, StoryObj } from '@storybook/react-vite'; import type { CheckRun } from '@/types'; +import { makePr, PanelFrame, withPrDetail } from './__fixtures__/pr-detail-data'; import { ChecksTab } from './ChecksTab'; -import { - makePr, - PanelFrame, - withPrDetail, -} from './__fixtures__/pr-detail-data'; const meta: Meta = { title: 'PR Detail/ChecksTab', component: ChecksTab, - decorators: [(Story) => ], + decorators: [ + (Story) => ( + + + + ), + ], }; export default meta; type Story = StoryObj; @@ -35,12 +37,54 @@ const allGreen: CheckRun[] = ['build', 'test', 'lint'].map((name, i) => ({ })); const mixed: CheckRun[] = [ - { id: 5001, name: 'CI / build', status: 'completed', conclusion: 'success', htmlUrl: '#', checkSuiteId: 9102 }, - { id: 5002, name: 'CI / test', status: 'completed', conclusion: 'success', htmlUrl: '#', checkSuiteId: 9102 }, - { id: 5003, name: 'CI / lint', status: 'completed', conclusion: 'failure', htmlUrl: '#', checkSuiteId: 9102 }, - { id: 5004, name: 'CI / typecheck', status: 'completed', conclusion: 'success', htmlUrl: '#', checkSuiteId: 9102 }, - { id: 5005, name: 'CI / e2e', status: 'completed', conclusion: 'failure', htmlUrl: '#', checkSuiteId: 9102 }, - { id: 5006, name: 'CI / docs', status: 'completed', conclusion: 'success', htmlUrl: '#', checkSuiteId: 9102 }, + { + id: 5001, + name: 'CI / build', + status: 'completed', + conclusion: 'success', + htmlUrl: '#', + checkSuiteId: 9102, + }, + { + id: 5002, + name: 'CI / test', + status: 'completed', + conclusion: 'success', + htmlUrl: '#', + checkSuiteId: 9102, + }, + { + id: 5003, + name: 'CI / lint', + status: 'completed', + conclusion: 'failure', + htmlUrl: '#', + checkSuiteId: 9102, + }, + { + id: 5004, + name: 'CI / typecheck', + status: 'completed', + conclusion: 'success', + htmlUrl: '#', + checkSuiteId: 9102, + }, + { + id: 5005, + name: 'CI / e2e', + status: 'completed', + conclusion: 'failure', + htmlUrl: '#', + checkSuiteId: 9102, + }, + { + id: 5006, + name: 'CI / docs', + status: 'completed', + conclusion: 'success', + htmlUrl: '#', + checkSuiteId: 9102, + }, ]; const allFailed: CheckRun[] = ['build', 'test', 'lint', 'typecheck', 'e2e', 'docs'].map( @@ -55,40 +99,43 @@ const allFailed: CheckRun[] = ['build', 'test', 'lint', 'typecheck', 'e2e', 'doc ); const allPendingPr = makePr({ - checks: allPending, overallStatus: 'yellow', pendingCheckNames: allPending.map((c) => c.name), passedCount: 0, + totalCheckCount: allPending.length, }); const allGreenPr = makePr({ - checks: allGreen, overallStatus: 'green', passedCount: allGreen.length, pendingCheckNames: [], + totalCheckCount: allGreen.length, }); const mixedPr = makePr({ - checks: mixed, overallStatus: 'red', passedCount: mixed.filter((c) => c.conclusion === 'success').length, failedCheckNames: mixed.filter((c) => c.conclusion === 'failure').map((c) => c.name), + failedCheckSuiteIds: mixed.filter((c) => c.conclusion === 'failure').map((c) => c.checkSuiteId), pendingCheckNames: [], + totalCheckCount: mixed.length, }); const allFailedPr = makePr({ - checks: allFailed, overallStatus: 'red', passedCount: 0, failedCheckNames: allFailed.map((c) => c.name), + failedCheckSuiteIds: allFailed.map((c) => c.checkSuiteId), pendingCheckNames: [], + totalCheckCount: allFailed.length, }); const noChecksPr = makePr({ - checks: [], overallStatus: 'gray', + failedCheckSuiteIds: [], passedCount: 0, pendingCheckNames: [], + totalCheckCount: 0, }); export const AllPending: Story = { diff --git a/src/BorgDock.Tauri/src/components/pr-detail/MergeReadinessChecklist.tsx b/src/BorgDock.Tauri/src/components/pr-detail/MergeReadinessChecklist.tsx index 8873d6bdb..5e97c7834 100644 --- a/src/BorgDock.Tauri/src/components/pr-detail/MergeReadinessChecklist.tsx +++ b/src/BorgDock.Tauri/src/components/pr-detail/MergeReadinessChecklist.tsx @@ -44,7 +44,7 @@ function scoreTone(score: number): 'success' | 'warning' | 'error' { function getCheckItems(pr: PullRequestWithChecks): ChecklistItem[] { // 1. Checks passed let checksStatus: CheckStatus; - if (pr.checks.length === 0) { + if (pr.totalCheckCount === 0) { checksStatus = 'none'; } else if (pr.failedCheckNames.length > 0) { checksStatus = 'fail'; @@ -55,11 +55,11 @@ function getCheckItems(pr: PullRequestWithChecks): ChecklistItem[] { } const checksDesc = - pr.checks.length === 0 + pr.totalCheckCount === 0 ? 'No CI checks configured' : pr.skippedCount > 0 - ? `${pr.passedCount}/${pr.checks.length - pr.skippedCount} passed (${pr.skippedCount} skipped)` - : `${pr.passedCount}/${pr.checks.length} checks passed`; + ? `${pr.passedCount}/${pr.totalCheckCount - pr.skippedCount} passed (${pr.skippedCount} skipped)` + : `${pr.passedCount}/${pr.totalCheckCount} checks passed`; // 2. Approved let reviewStatus: CheckStatus; @@ -132,12 +132,7 @@ const CheckGlyph = () => ( ); const XGlyph = () => ( ); const ClockGlyph = () => ( @@ -229,9 +224,7 @@ export function MergeReadinessChecklist({ pr }: MergeReadinessChecklistProps) { key={item.label} className={ 'flex items-center gap-2.5 px-3.5 py-2' + - (idx < items.length - 1 - ? ' border-b border-[var(--color-subtle-border)]' - : '') + (idx < items.length - 1 ? ' border-b border-[var(--color-subtle-border)]' : '') } data-merge-check={item.label} > diff --git a/src/BorgDock.Tauri/src/components/pr-detail/PRDetailApp.stories.tsx b/src/BorgDock.Tauri/src/components/pr-detail/PRDetailApp.stories.tsx index d8012d021..83f39ac6c 100644 --- a/src/BorgDock.Tauri/src/components/pr-detail/PRDetailApp.stories.tsx +++ b/src/BorgDock.Tauri/src/components/pr-detail/PRDetailApp.stories.tsx @@ -1,18 +1,25 @@ // src/components/pr-detail/PRDetailApp.stories.tsx import type { Meta, StoryObj } from '@storybook/react-vite'; -import { PrDetailApp } from './PRDetailApp'; import { openPr, + openPrChecks, PanelFrame, SETTINGS_BASELINE, withPrDetail, } from './__fixtures__/pr-detail-data'; +import { PrDetailApp } from './PRDetailApp'; const meta: Meta = { title: 'PR Detail/PRDetailApp', component: PrDetailApp, - decorators: [(Story) => ], + decorators: [ + (Story) => ( + + + + ), + ], }; export default meta; type Story = StoryObj; @@ -29,7 +36,7 @@ export const Default: Story = { invokeResponses: baseInvokes, githubResponses: { getOpenPRs: [openPr.pullRequest], - getCheckRunsForRef: openPr.checks, + getCheckRunsForRef: openPrChecks, }, }), ], @@ -53,7 +60,7 @@ export const MissingParams: Story = { injectedPrParams: null, githubResponses: { getOpenPRs: [openPr.pullRequest], - getCheckRunsForRef: openPr.checks, + getCheckRunsForRef: openPrChecks, }, }), ], @@ -79,7 +86,7 @@ export const LoadSettingsRejects: Story = { }, githubResponses: { getOpenPRs: [openPr.pullRequest], - getCheckRunsForRef: openPr.checks, + getCheckRunsForRef: openPrChecks, }, }), ], diff --git a/src/BorgDock.Tauri/src/components/pr-detail/PRDetailApp.tsx b/src/BorgDock.Tauri/src/components/pr-detail/PRDetailApp.tsx index 645a17917..0b677134d 100644 --- a/src/BorgDock.Tauri/src/components/pr-detail/PRDetailApp.tsx +++ b/src/BorgDock.Tauri/src/components/pr-detail/PRDetailApp.tsx @@ -21,6 +21,9 @@ const XIcon = () => ( export function PrDetailApp() { const [pr, setPr] = useState(null); + // Raw check runs for the Checks tab — fetched here (REST cold path) and + // refreshed via PR_REFRESHED_EVENT; the PR type itself only carries counts. + const [localChecks, setLocalChecks] = useState([]); const prRef = useRef(null); const [isLoading, setIsLoading] = useState(true); const [error, setError] = useState(null); @@ -143,6 +146,7 @@ export function PrDetailApp() { if (cancelled) return; const prWithChecks = aggregatePrWithChecks(targetPr, checks); setPr(prWithChecks); + setLocalChecks(checks); // Update window title getCurrentWindow() @@ -176,6 +180,8 @@ export function PrDetailApp() { } if (detail.pr) { setPr(detail.pr); + // Optimistic refreshes (e.g. mark-merged) carry no checks — keep ours. + if (detail.checks) setLocalChecks(detail.checks); getCurrentWindow() .setTitle(`PR #${number} - ${detail.pr.pullRequest.title}`) .catch(console.debug); /* fire-and-forget */ @@ -249,7 +255,7 @@ export function PrDetailApp() { return (
- +
); diff --git a/src/BorgDock.Tauri/src/components/pr-detail/PRDetailPanel.tsx b/src/BorgDock.Tauri/src/components/pr-detail/PRDetailPanel.tsx index c736e8457..ca9016c11 100644 --- a/src/BorgDock.Tauri/src/components/pr-detail/PRDetailPanel.tsx +++ b/src/BorgDock.Tauri/src/components/pr-detail/PRDetailPanel.tsx @@ -1,15 +1,15 @@ import { getCurrentWindow } from '@tauri-apps/api/window'; import { useCallback, useEffect, useState } from 'react'; -import { createLogger } from '@/services/logger'; -import { WindowControls } from '@/components/shared/chrome'; import { ConfirmDialog } from '@/components/shared/ConfirmDialog'; -import { Avatar, IconButton, Pill, Ring, Tabs, TitleBar } from '@/components/shared/primitives'; +import { WindowControls } from '@/components/shared/chrome'; import type { TabDef } from '@/components/shared/primitives'; +import { Avatar, IconButton, Pill, Ring, Tabs, TitleBar } from '@/components/shared/primitives'; +import { createLogger } from '@/services/logger'; import { computeMergeScore } from '@/services/merge-score'; import { openPrDetail } from '@/services/windows'; -import { useUiStore } from '@/stores/ui-store'; import { usePrDetailJumpStore } from '@/stores/pr-detail-jump-store'; -import type { PullRequestWithChecks } from '@/types'; +import { useUiStore } from '@/stores/ui-store'; +import type { CheckRun, PullRequestWithChecks } from '@/types'; import { ActionBar } from './ActionBar'; import { ActivityStrip } from './ActivityStrip'; import { CheckoutPanel } from './CheckoutPanel'; @@ -92,7 +92,6 @@ const BorgDockLogo = () => ( ); - const BranchIcon = () => ( ( ); const HeaderSpinnerIcon = () => ( -
{/* Merge readiness gauge */} - +
{/* Status pills row */} @@ -358,11 +363,7 @@ export function PrDetailPanel({ pr, popOutWindow }: PrDetailPanelProps) { {!isMerged && p.state === 'closed' && Closed} {!isTerminal && p.mergeable === true && Mergeable} {!isTerminal && p.mergeable === false && Conflicts} - {!isTerminal && totalChecks > 0 && ( - - {passedCount} passed - - )} + {!isTerminal && totalChecks > 0 && {passedCount} passed} {!isTerminal && pr.pendingCheckNames.length > 0 && ( }> {pr.pendingCheckNames.length} running @@ -380,17 +381,23 @@ export function PrDetailPanel({ pr, popOutWindow }: PrDetailPanelProps) { {/* Author + date + branches */}
- {p.authorLogin} + + {p.authorLogin} + · {formatDate(p.createdAt)} · - {formatAge(p.createdAt)} old + + {formatAge(p.createdAt)} old + · {p.headRef} - {p.baseRef} + + {p.baseRef} +
@@ -398,15 +405,21 @@ export function PrDetailPanel({ pr, popOutWindow }: PrDetailPanelProps) {
+{p.additions} −{p.deletions} - · + + · + {p.changedFiles} file{p.changedFiles !== 1 ? 's' : ''} - · + + · + {p.commitCount} commit{p.commitCount !== 1 ? 's' : ''} - · + + · + {p.commentCount} comment{p.commentCount !== 1 ? 's' : ''} @@ -427,15 +440,10 @@ export function PrDetailPanel({ pr, popOutWindow }: PrDetailPanelProps) {
{/* Action bar — sticky, all PR actions */} - + {/* Activity strip — persistent checks summary, click-to-jump */} - {pr.checks.length > 0 && ( + {pr.totalCheckCount > 0 && (
- setActiveTab(id as Tab)} - tabs={tabDefs} - /> + setActiveTab(id as Tab)} tabs={tabDefs} />
{/* Tab content — tabs mount lazily on first activation, cached afterwards */} @@ -484,7 +488,7 @@ export function PrDetailPanel({ pr, popOutWindow }: PrDetailPanelProps) { )} {mountedTabs.has('Checks') && (
- +
)} {mountedTabs.has('Discussion') && ( @@ -495,9 +499,7 @@ export function PrDetailPanel({ pr, popOutWindow }: PrDetailPanelProps) { repoName={pr.pullRequest.repoName} prUpdatedAt={pr.pullRequest.updatedAt} onJumpToFile={(target) => { - usePrDetailJumpStore - .getState() - .setJumpTarget({ ...target, ts: Date.now() }); + usePrDetailJumpStore.getState().setJumpTarget({ ...target, ts: Date.now() }); setActiveTab('Files'); }} /> diff --git a/src/BorgDock.Tauri/src/components/pr-detail/__fixtures__/pr-detail-data.tsx b/src/BorgDock.Tauri/src/components/pr-detail/__fixtures__/pr-detail-data.tsx index add423121..1c6c64299 100644 --- a/src/BorgDock.Tauri/src/components/pr-detail/__fixtures__/pr-detail-data.tsx +++ b/src/BorgDock.Tauri/src/components/pr-detail/__fixtures__/pr-detail-data.tsx @@ -1,27 +1,20 @@ // src/components/pr-detail/__fixtures__/pr-detail-data.tsx -import type { ReactNode } from 'react'; import type { Decorator } from '@storybook/react-vite'; +import type { ReactNode } from 'react'; +import { usePrDetailJumpStore } from '@/stores/pr-detail-jump-store'; import { useSettingsStore } from '@/stores/settings-store'; import { useUiStore } from '@/stores/ui-store'; -import { usePrDetailJumpStore } from '@/stores/pr-detail-jump-store'; -import type { - AppSettings, - CheckRun, - PullRequest, - PullRequestWithChecks, -} from '@/types'; +import type { AppSettings, CheckRun, PullRequest, PullRequestWithChecks } from '@/types'; import { - getControl, type GithubResponses, + getControl, type PrActionResponses, } from '../../../../.storybook/mocks/control'; // ── Deep-merge helper ───────────────────────────────────────── -type DeepPartial = T extends object - ? { [K in keyof T]?: DeepPartial } - : T; +type DeepPartial = T extends object ? { [K in keyof T]?: DeepPartial } : T; function deepMerge(base: T, over: DeepPartial | undefined): T { if (!over) return base; @@ -80,19 +73,22 @@ const BASE_CHECKS: CheckRun[] = [ }, ]; +// Exposed so stories can mock the REST check-run fetch (getCheckRunsForRef) +// for the open PR — the type no longer carries raw checks inline. +export const openPrChecks: CheckRun[] = BASE_CHECKS; + const BASE: PullRequestWithChecks = { pullRequest: BASE_PR, - checks: BASE_CHECKS, overallStatus: 'yellow', failedCheckNames: [], + failedCheckSuiteIds: [], pendingCheckNames: ['CI / build', 'CI / test'], passedCount: 0, skippedCount: 0, + totalCheckCount: 2, }; -export function makePr( - overrides?: DeepPartial, -): PullRequestWithChecks { +export function makePr(overrides?: DeepPartial): PullRequestWithChecks { return deepMerge(BASE, overrides); } @@ -108,25 +104,52 @@ export const draftPr: PullRequestWithChecks = makePr({ }, overallStatus: 'gray', pendingCheckNames: [], - checks: [], + totalCheckCount: 0, }); +const APPROVED_CHECKS: CheckRun[] = [ + { + id: 2001, + name: 'CI / build', + status: 'completed', + conclusion: 'success', + htmlUrl: '#', + checkSuiteId: 9001, + }, + { + id: 2002, + name: 'CI / test', + status: 'completed', + conclusion: 'success', + htmlUrl: '#', + checkSuiteId: 9001, + }, + { + id: 2003, + name: 'CI / lint', + status: 'completed', + conclusion: 'success', + htmlUrl: '#', + checkSuiteId: 9001, + }, +]; + +// Exposed so stories can mock the REST check-run fetch for the approved PR. +export const approvedPrChecks: CheckRun[] = APPROVED_CHECKS; + const APPROVED_REVIEW_PR: PullRequestWithChecks = { pullRequest: { ...BASE_PR, reviewStatus: 'approved', commentCount: 4, }, - checks: [ - { id: 2001, name: 'CI / build', status: 'completed', conclusion: 'success', htmlUrl: '#', checkSuiteId: 9001 }, - { id: 2002, name: 'CI / test', status: 'completed', conclusion: 'success', htmlUrl: '#', checkSuiteId: 9001 }, - { id: 2003, name: 'CI / lint', status: 'completed', conclusion: 'success', htmlUrl: '#', checkSuiteId: 9001 }, - ], overallStatus: 'green', failedCheckNames: [], + failedCheckSuiteIds: [], pendingCheckNames: [], passedCount: 3, skippedCount: 0, + totalCheckCount: 3, }; export const approvedPr: PullRequestWithChecks = APPROVED_REVIEW_PR; @@ -148,7 +171,7 @@ export const mergedPr: PullRequestWithChecks = makePr({ overallStatus: 'green', passedCount: 3, pendingCheckNames: [], - checks: APPROVED_REVIEW_PR.checks, + totalCheckCount: 3, }); export const closedPr: PullRequestWithChecks = makePr({ @@ -166,9 +189,9 @@ export const mergeConflictPr: PullRequestWithChecks = makePr({ reviewStatus: 'approved', }, overallStatus: 'green', - checks: APPROVED_REVIEW_PR.checks, passedCount: 3, pendingCheckNames: [], + totalCheckCount: 3, }); export const staleChecksPr: PullRequestWithChecks = makePr({ @@ -219,8 +242,7 @@ export interface WithPrDetailOptions { // fall back to an empty object cast as AppSettings (the section/tab // stories will rarely depend on settings beyond `repos`). export const SETTINGS_BASELINE: AppSettings = - (useSettingsStore.getState().settings as AppSettings | undefined) ?? - ({} as AppSettings); + (useSettingsStore.getState().settings as AppSettings | undefined) ?? ({} as AppSettings); const UI_BASELINE = useUiStore.getState(); const JUMP_BASELINE = usePrDetailJumpStore.getState(); @@ -240,7 +262,11 @@ export function withPrDetail( const params = options.injectedPrParams ?? (pr - ? { owner: pr.pullRequest.repoOwner, repo: pr.pullRequest.repoName, number: pr.pullRequest.number } + ? { + owner: pr.pullRequest.repoOwner, + repo: pr.pullRequest.repoName, + number: pr.pullRequest.number, + } : { owner: 'borght-dev', repo: 'BorgDock', number: 1 }); (window as unknown as Record).__BORGDOCK_PR_DETAIL__ = params; } diff --git a/src/BorgDock.Tauri/src/components/pr-detail/__tests__/ChecksTab.test.tsx b/src/BorgDock.Tauri/src/components/pr-detail/__tests__/ChecksTab.test.tsx index e63ed97d9..270a86e2c 100644 --- a/src/BorgDock.Tauri/src/components/pr-detail/__tests__/ChecksTab.test.tsx +++ b/src/BorgDock.Tauri/src/components/pr-detail/__tests__/ChecksTab.test.tsx @@ -235,9 +235,7 @@ describe('ChecksTab', () => { makeCheck({ id: 2, name: 'ci/test', conclusion: 'failure', checkSuiteId: 100 }), ]; const { container } = render(); - expect( - container.querySelector('[data-check-row][data-check-state="failed"]'), - ).toBeTruthy(); + expect(container.querySelector('[data-check-row][data-check-state="failed"]')).toBeTruthy(); }); it('renders LinearProgress in the summary', () => { @@ -313,15 +311,15 @@ describe('ChecksTab', () => { checks: [], overallStatus: 'red', failedCheckNames: ['ci/test'], + failedCheckSuiteIds: [], pendingCheckNames: [], passedCount: 1, skippedCount: 0, + totalCheckCount: 2, }; // biome-ignore lint/suspicious/noExplicitAny: fixture-only any cast for narrow type const { container } = render(); - const failedRow = container.querySelector( - '[data-check-row][data-check-state="failed"]', - ); + const failedRow = container.querySelector('[data-check-row][data-check-state="failed"]'); expect(failedRow).toBeTruthy(); expect(failedRow?.querySelector('.bd-btn')).toBeTruthy(); }); diff --git a/src/BorgDock.Tauri/src/components/pr-detail/__tests__/MergeReadinessChecklist.test.tsx b/src/BorgDock.Tauri/src/components/pr-detail/__tests__/MergeReadinessChecklist.test.tsx index 70fa8542d..96b63a759 100644 --- a/src/BorgDock.Tauri/src/components/pr-detail/__tests__/MergeReadinessChecklist.test.tsx +++ b/src/BorgDock.Tauri/src/components/pr-detail/__tests__/MergeReadinessChecklist.test.tsx @@ -1,6 +1,6 @@ import { cleanup, render, screen } from '@testing-library/react'; import { afterEach, describe, expect, it } from 'vitest'; -import type { CheckRun, PullRequestWithChecks } from '@/types'; +import type { PullRequestWithChecks } from '@/types'; import { MergeReadinessChecklist } from '../MergeReadinessChecklist'; function makePr(overrides: Partial = {}): PullRequestWithChecks { @@ -30,24 +30,13 @@ function makePr(overrides: Partial = {}): PullRequestWith commitCount: 1, requestedReviewers: [], }, - checks: [], overallStatus: 'green', failedCheckNames: [], + failedCheckSuiteIds: [], pendingCheckNames: [], passedCount: 0, skippedCount: 0, - ...overrides, - }; -} - -function makeCheck(overrides: Partial = {}): CheckRun { - return { - id: 1, - name: 'build', - status: 'completed', - conclusion: 'success', - htmlUrl: 'https://github.com/runs/1', - checkSuiteId: 100, + totalCheckCount: 0, ...overrides, }; } @@ -72,9 +61,9 @@ describe('MergeReadinessChecklist', () => { it('shows 100 score for fully ready PR', () => { const pr = makePr({ - checks: [makeCheck()], passedCount: 1, skippedCount: 0, + totalCheckCount: 1, pullRequest: { ...makePr().pullRequest, reviewStatus: 'approved', @@ -100,8 +89,8 @@ describe('MergeReadinessChecklist', () => { it('shows checks count description', () => { const pr = makePr({ - checks: [makeCheck({ id: 1 }), makeCheck({ id: 2 })], passedCount: 2, + totalCheckCount: 2, }); render(); expect(screen.getByText('2/2 checks passed')).toBeTruthy(); @@ -109,9 +98,9 @@ describe('MergeReadinessChecklist', () => { it('includes skipped count in description', () => { const pr = makePr({ - checks: [makeCheck({ id: 1 }), makeCheck({ id: 2, conclusion: 'skipped' })], passedCount: 1, skippedCount: 1, + totalCheckCount: 2, }); render(); expect(screen.getByText('1/1 passed (1 skipped)')).toBeTruthy(); @@ -190,9 +179,10 @@ describe('MergeReadinessChecklist', () => { it('shows reduced score for failed checks', () => { const pr = makePr({ - checks: [makeCheck({ id: 1, conclusion: 'failure' })], passedCount: 0, failedCheckNames: ['build'], + failedCheckSuiteIds: [100], + totalCheckCount: 1, pullRequest: { ...makePr().pullRequest, reviewStatus: 'approved', @@ -206,9 +196,10 @@ describe('MergeReadinessChecklist', () => { it('shows 0 score for worst-case PR', () => { const pr = makePr({ - checks: [makeCheck({ id: 1, conclusion: 'failure' })], passedCount: 0, failedCheckNames: ['build'], + failedCheckSuiteIds: [100], + totalCheckCount: 1, pullRequest: { ...makePr().pullRequest, reviewStatus: 'none', diff --git a/src/BorgDock.Tauri/src/components/pr-detail/__tests__/OverviewTab.test.tsx b/src/BorgDock.Tauri/src/components/pr-detail/__tests__/OverviewTab.test.tsx index ed5d555f4..5c73554c7 100644 --- a/src/BorgDock.Tauri/src/components/pr-detail/__tests__/OverviewTab.test.tsx +++ b/src/BorgDock.Tauri/src/components/pr-detail/__tests__/OverviewTab.test.tsx @@ -78,12 +78,13 @@ function makePr(overrides: Partial = {}): PullRequestWith commitCount: 3, requestedReviewers: [], }, - checks: [], overallStatus: 'green', failedCheckNames: [], + failedCheckSuiteIds: [], pendingCheckNames: [], passedCount: 0, skippedCount: 0, + totalCheckCount: 0, ...overrides, }; } @@ -98,7 +99,14 @@ describe('OverviewTab', () => { useSettingsStore.setState({ settings: { ...useSettingsStore.getState().settings, - claudeApi: { model: 'claude-sonnet-4-6', maxTokens: 1024, prSummaryEnabled: true, diffExplanationsEnabled: true, reviewNudgePhrasingEnabled: false, commitMessageSuggestionsEnabled: false }, + claudeApi: { + model: 'claude-sonnet-4-6', + maxTokens: 1024, + prSummaryEnabled: true, + diffExplanationsEnabled: true, + reviewNudgePhrasingEnabled: false, + commitMessageSuggestionsEnabled: false, + }, }, }); }); @@ -137,7 +145,14 @@ describe('OverviewTab', () => { useSettingsStore.setState({ settings: { ...useSettingsStore.getState().settings, - claudeApi: { model: 'claude-sonnet-4-6', maxTokens: 1024, prSummaryEnabled: true, diffExplanationsEnabled: true, reviewNudgePhrasingEnabled: false, commitMessageSuggestionsEnabled: false }, + claudeApi: { + model: 'claude-sonnet-4-6', + maxTokens: 1024, + prSummaryEnabled: true, + diffExplanationsEnabled: true, + reviewNudgePhrasingEnabled: false, + commitMessageSuggestionsEnabled: false, + }, }, }); render(); @@ -150,7 +165,15 @@ describe('OverviewTab', () => { useSettingsStore.setState({ settings: { ...useSettingsStore.getState().settings, - claudeApi: { apiKey: 'sk-test', model: 'claude-sonnet-4-6', maxTokens: 1024, prSummaryEnabled: true, diffExplanationsEnabled: true, reviewNudgePhrasingEnabled: false, commitMessageSuggestionsEnabled: false }, + claudeApi: { + apiKey: 'sk-test', + model: 'claude-sonnet-4-6', + maxTokens: 1024, + prSummaryEnabled: true, + diffExplanationsEnabled: true, + reviewNudgePhrasingEnabled: false, + commitMessageSuggestionsEnabled: false, + }, }, }); render(); @@ -161,7 +184,15 @@ describe('OverviewTab', () => { useSettingsStore.setState({ settings: { ...useSettingsStore.getState().settings, - claudeApi: { apiKey: 'sk-test', model: 'claude-sonnet-4-6', maxTokens: 1024, prSummaryEnabled: true, diffExplanationsEnabled: true, reviewNudgePhrasingEnabled: false, commitMessageSuggestionsEnabled: false }, + claudeApi: { + apiKey: 'sk-test', + model: 'claude-sonnet-4-6', + maxTokens: 1024, + prSummaryEnabled: true, + diffExplanationsEnabled: true, + reviewNudgePhrasingEnabled: false, + commitMessageSuggestionsEnabled: false, + }, }, }); diff --git a/src/BorgDock.Tauri/src/components/pr-detail/__tests__/PRDetailApp.test.tsx b/src/BorgDock.Tauri/src/components/pr-detail/__tests__/PRDetailApp.test.tsx index 3508d2681..a104d764c 100644 --- a/src/BorgDock.Tauri/src/components/pr-detail/__tests__/PRDetailApp.test.tsx +++ b/src/BorgDock.Tauri/src/components/pr-detail/__tests__/PRDetailApp.test.tsx @@ -37,14 +37,15 @@ vi.mock('@/components/shared/WindowTitleBar', () => ({ })); vi.mock('@/services/github/aggregate', () => ({ - aggregatePrWithChecks: (pr: unknown, checks: unknown) => ({ + aggregatePrWithChecks: (pr: unknown, _checks: unknown) => ({ pullRequest: pr, - checks, overallStatus: 'green', failedCheckNames: [], + failedCheckSuiteIds: [], pendingCheckNames: [], passedCount: 0, skippedCount: 0, + totalCheckCount: 0, }), })); @@ -234,9 +235,7 @@ describe('PrDetailApp', () => { // wait a tick for any pending setState await new Promise((r) => setTimeout(r, 50)); - expect(consoleErrSpy).not.toHaveBeenCalledWith( - expect.stringContaining('not wrapped in act'), - ); + expect(consoleErrSpy).not.toHaveBeenCalledWith(expect.stringContaining('not wrapped in act')); consoleErrSpy.mockRestore(); }); diff --git a/src/BorgDock.Tauri/src/components/pr-detail/__tests__/PRDetailPanel.test.tsx b/src/BorgDock.Tauri/src/components/pr-detail/__tests__/PRDetailPanel.test.tsx index cdc6f4501..e8c3bc83c 100644 --- a/src/BorgDock.Tauri/src/components/pr-detail/__tests__/PRDetailPanel.test.tsx +++ b/src/BorgDock.Tauri/src/components/pr-detail/__tests__/PRDetailPanel.test.tsx @@ -92,14 +92,18 @@ interface FakePrOptions { passedCount?: number; failedCheckNames?: string[]; skippedCount?: number; + checks?: CheckRun[]; } -function makePr(overrides: Partial & FakePrOptions = {}): PullRequestWithChecks { +function makePr( + overrides: Partial & FakePrOptions = {}, +): PullRequestWithChecks { const { pendingCheckNames = [], passedCount = 0, failedCheckNames = [], skippedCount = 0, + checks: checksOverride, ...rest } = overrides; @@ -118,6 +122,8 @@ function makePr(overrides: Partial & FakePrOptions = {}): return { ...stubCheck, id: i + 1, name }; }); + const finalChecks = checksOverride ?? checks; + return { pullRequest: { number: 42, @@ -147,11 +153,14 @@ function makePr(overrides: Partial & FakePrOptions = {}): }, overallStatus: 'green', ...rest, - checks: rest.checks ?? checks, failedCheckNames, + failedCheckSuiteIds: finalChecks + .filter((c: CheckRun) => c.conclusion === 'failure' || c.conclusion === 'timed_out') + .map((c: CheckRun) => c.checkSuiteId), pendingCheckNames, passedCount, skippedCount, + totalCheckCount: finalChecks.length, }; } @@ -294,26 +303,66 @@ describe('PrDetailPanel', () => { describe('terminal state pills', () => { it('renders a Merged pill when mergedAt is set', () => { - render(); + render( + , + ); const pill = screen.getByText('Merged'); expect(pill.classList.contains('bd-pill--merged')).toBe(true); }); it('renders a Closed pill when state is closed and not merged', () => { - render(); + render( + , + ); const pill = screen.getByText('Closed'); expect(pill.classList.contains('bd-pill--neutral')).toBe(true); }); it('suppresses the Mergeable pill in terminal state', () => { - render(); + render( + , + ); expect(screen.queryByText('Mergeable')).toBeNull(); }); it('suppresses the passed-count pill in terminal state', () => { const merged = makePr({ pullRequest: { ...makePr().pullRequest, state: 'closed', mergedAt: '2026-05-06T12:00:00Z' }, - checks: [{ id: 1, name: 'ci', status: 'completed', conclusion: 'success', htmlUrl: '', checkSuiteId: 1 }], + checks: [ + { + id: 1, + name: 'ci', + status: 'completed', + conclusion: 'success', + htmlUrl: '', + checkSuiteId: 1, + }, + ], passedCount: 1, }); render(); diff --git a/src/BorgDock.Tauri/src/components/pr-detail/__tests__/usePrActions.test.tsx b/src/BorgDock.Tauri/src/components/pr-detail/__tests__/usePrActions.test.tsx index 611b0bbc9..2570dbbe0 100644 --- a/src/BorgDock.Tauri/src/components/pr-detail/__tests__/usePrActions.test.tsx +++ b/src/BorgDock.Tauri/src/components/pr-detail/__tests__/usePrActions.test.tsx @@ -1,8 +1,7 @@ import { act, renderHook } from '@testing-library/react'; import { describe, expect, it, vi } from 'vitest'; - -import { usePrActions } from '../usePrActions'; import type { PullRequestWithChecks } from '@/types'; +import { usePrActions } from '../usePrActions'; vi.mock('@/services/pr-actions', () => ({ mergePr: vi.fn(async () => true), @@ -34,7 +33,9 @@ vi.mock('@/stores/settings-store', () => ({ ), })); -function fakePr(overrides: Partial = {}): PullRequestWithChecks { +function fakePr( + overrides: Partial = {}, +): PullRequestWithChecks { return { pullRequest: { number: 1, @@ -61,12 +62,13 @@ function fakePr(overrides: Partial = {}): requestedReviewers: [], ...overrides, }, - checks: [], overallStatus: 'green', failedCheckNames: [], + failedCheckSuiteIds: [], pendingCheckNames: [], passedCount: 0, skippedCount: 0, + totalCheckCount: 0, }; } @@ -80,9 +82,7 @@ describe('usePrActions', () => { }); it('isReady=true for green + approved + non-draft + mergeable', () => { - const { result } = renderHook(() => - usePrActions(fakePr({ mergeable: true })), - ); + const { result } = renderHook(() => usePrActions(fakePr({ mergeable: true }))); expect(result.current.isReady).toBe(true); }); diff --git a/src/BorgDock.Tauri/src/components/pr/MultiSignalIndicator.tsx b/src/BorgDock.Tauri/src/components/pr/MultiSignalIndicator.tsx index f8fec6486..21101be3a 100644 --- a/src/BorgDock.Tauri/src/components/pr/MultiSignalIndicator.tsx +++ b/src/BorgDock.Tauri/src/components/pr/MultiSignalIndicator.tsx @@ -30,7 +30,7 @@ function arcPath(cx: number, cy: number, r: number, startAngle: number, endAngle } function computeCiColor(pr: PullRequestWithChecks): SignalColor { - if (pr.checks.length === 0) return 'gray'; + if (pr.totalCheckCount === 0) return 'gray'; if (pr.failedCheckNames.length > 0) return 'red'; if (pr.pendingCheckNames.length > 0) return 'yellow'; return 'green'; diff --git a/src/BorgDock.Tauri/src/components/pr/PrCardContainer.tsx b/src/BorgDock.Tauri/src/components/pr/PrCardContainer.tsx index c632a1a37..36855983e 100644 --- a/src/BorgDock.Tauri/src/components/pr/PrCardContainer.tsx +++ b/src/BorgDock.Tauri/src/components/pr/PrCardContainer.tsx @@ -3,11 +3,7 @@ import { PriorityReasonLabel } from '@/components/focus/PriorityReasonLabel'; import { LinkedWorkItemBadge } from '@/components/pr-detail/LinkedWorkItemBadge'; import { usePrCardActions } from '@/hooks/usePrCardActions'; import { computeMergeScore } from '@/services/merge-score'; -import { - type PrActionId, - primaryFor, - shapeFromPrWithChecks, -} from '@/services/pr-action-resolver'; +import { type PrActionId, primaryFor, shapeFromPrWithChecks } from '@/services/pr-action-resolver'; import type { PriorityFactor } from '@/services/priority-scoring'; import { openPrDetail } from '@/services/windows'; import { detectWorkItemIds } from '@/services/work-item-linker'; @@ -16,7 +12,7 @@ import { useUiStore } from '@/stores/ui-store'; import type { PullRequestWithChecks } from '@/types'; import { ConfirmDialog } from '../shared/ConfirmDialog'; import { HoverActionPillBar } from './HoverActionPillBar'; -import { PrCardView, type PrCardData } from './PrCardView'; +import { type PrCardData, PrCardView } from './PrCardView'; import { PrContextMenu } from './PrContextMenu'; interface PrCardContainerProps { @@ -34,7 +30,7 @@ function mapToPrCardData( const pr = prw.pullRequest; const failedCount = prw.failedCheckNames.length; const pendingCount = prw.pendingCheckNames.length; - const totalChecks = prw.checks.length; + const totalChecks = prw.totalCheckCount; const relevant = totalChecks - prw.skippedCount; const statusLabel = failedCount > 0 diff --git a/src/BorgDock.Tauri/src/components/pr/PrContextMenu.tsx b/src/BorgDock.Tauri/src/components/pr/PrContextMenu.tsx index 6fdcb0c3a..f05a055e8 100644 --- a/src/BorgDock.Tauri/src/components/pr/PrContextMenu.tsx +++ b/src/BorgDock.Tauri/src/components/pr/PrContextMenu.tsx @@ -1,12 +1,7 @@ import { useCallback, useEffect, useLayoutEffect, useRef, useState } from 'react'; import { useClaudeActions } from '@/hooks/useClaudeActions'; import { sendOsNotification } from '@/services/notification'; -import { - checkoutPrBranch, - mergePr, - openPrInBrowser, - rerunChecks, -} from '@/services/pr-actions'; +import { checkoutPrBranch, mergePr, openPrInBrowser, rerunChecks } from '@/services/pr-actions'; import { findRepoConfig } from '@/services/repo-lookup'; import { openPrDetail } from '@/services/windows'; import { useSettingsStore } from '@/stores/settings-store'; @@ -109,10 +104,8 @@ export function PrContextMenu({ pr, position, onClose, onConfirmAction }: PrCont const canMerge = !pullRequest.isDraft && overallStatus === 'green' && pullRequest.state === 'open'; - // Find a failed check's run ID for rerun (pick the first failed check's suite) - const failedCheck = pr.checks.find( - (c) => c.conclusion === 'failure' || c.conclusion === 'timed_out', - ); + // Pick the first failed check's suite for rerun. + const failedSuiteId: number | undefined = pr.failedCheckSuiteIds[0]; const handleAction = useCallback( (action: () => Promise, errorTitle?: string) => { @@ -166,8 +159,8 @@ export function PrContextMenu({ pr, position, onClose, onConfirmAction }: PrCont }, [onClose, onConfirmAction]); const handleRerunFailed = handleAction(async () => { - if (!failedCheck) return; - await rerunChecks({ repoOwner: owner, repoName: repo, checkSuiteId: failedCheck.checkSuiteId }); + if (failedSuiteId === undefined) return; + await rerunChecks({ repoOwner: owner, repoName: repo, checkSuiteId: failedSuiteId }); }, 'Failed to re-run checks'); const handleFixWithClaude = handleAction(async () => { @@ -261,7 +254,7 @@ export function PrContextMenu({ pr, position, onClose, onConfirmAction }: PrCont diff --git a/src/BorgDock.Tauri/src/components/pr/__tests__/MultiSignalIndicator.test.tsx b/src/BorgDock.Tauri/src/components/pr/__tests__/MultiSignalIndicator.test.tsx index f7221ecd7..97c9652ba 100644 --- a/src/BorgDock.Tauri/src/components/pr/__tests__/MultiSignalIndicator.test.tsx +++ b/src/BorgDock.Tauri/src/components/pr/__tests__/MultiSignalIndicator.test.tsx @@ -1,11 +1,15 @@ import { cleanup, render, screen } from '@testing-library/react'; import { afterEach, describe, expect, it } from 'vitest'; -import type { PullRequestWithChecks } from '@/types'; +import type { CheckRun, PullRequestWithChecks } from '@/types'; import { MultiSignalIndicator } from '../MultiSignalIndicator'; afterEach(cleanup); -function makePr(overrides: Partial = {}): PullRequestWithChecks { +function makePr( + overrides: Partial & { checks?: CheckRun[] } = {}, +): PullRequestWithChecks { + const { checks: checksOverride, ...rest } = overrides; + const checks = checksOverride ?? []; return { pullRequest: { number: 1, @@ -31,14 +35,19 @@ function makePr(overrides: Partial = {}): PullRequestWith changedFiles: 2, commitCount: 1, requestedReviewers: [], - ...overrides.pullRequest, + ...rest.pullRequest, }, - checks: overrides.checks ?? [], - overallStatus: overrides.overallStatus ?? 'green', - failedCheckNames: overrides.failedCheckNames ?? [], - pendingCheckNames: overrides.pendingCheckNames ?? [], - passedCount: overrides.passedCount ?? 0, - skippedCount: overrides.skippedCount ?? 0, + overallStatus: rest.overallStatus ?? 'green', + failedCheckNames: rest.failedCheckNames ?? [], + failedCheckSuiteIds: + rest.failedCheckSuiteIds ?? + checks + .filter((c: CheckRun) => c.conclusion === 'failure' || c.conclusion === 'timed_out') + .map((c: CheckRun) => c.checkSuiteId), + pendingCheckNames: rest.pendingCheckNames ?? [], + passedCount: rest.passedCount ?? 0, + skippedCount: rest.skippedCount ?? 0, + totalCheckCount: rest.totalCheckCount ?? checks.length, }; } diff --git a/src/BorgDock.Tauri/src/components/pr/__tests__/PrCardContainer.click.test.tsx b/src/BorgDock.Tauri/src/components/pr/__tests__/PrCardContainer.click.test.tsx index 281e29f34..fd4f88599 100644 --- a/src/BorgDock.Tauri/src/components/pr/__tests__/PrCardContainer.click.test.tsx +++ b/src/BorgDock.Tauri/src/components/pr/__tests__/PrCardContainer.click.test.tsx @@ -52,7 +52,9 @@ vi.mock('@/stores/ui-store', () => { }); vi.mock('@/stores/pr-store', () => { - const fn = vi.fn() as unknown as ReturnType & { getState: ReturnType }; + const fn = vi.fn() as unknown as ReturnType & { + getState: ReturnType; + }; fn.mockImplementation((selector: (state: Record) => unknown) => selector({ username: 'testuser' }), ); @@ -99,12 +101,13 @@ function makePr(): PullRequestWithChecks { commitCount: 1, requestedReviewers: [], }, - checks: [], overallStatus: 'green', failedCheckNames: [], + failedCheckSuiteIds: [], pendingCheckNames: [], passedCount: 0, skippedCount: 0, + totalCheckCount: 0, }; } @@ -134,10 +137,7 @@ describe('PrCardContainer card-click → pop-out detail', () => { const moreBtn = container.querySelector('[data-pr-action="more"]') as HTMLElement; expect(moreBtn).toBeInTheDocument(); fireEvent.click(moreBtn); - expect(invokeMock).not.toHaveBeenCalledWith( - 'open_pr_detail_window', - expect.anything(), - ); + expect(invokeMock).not.toHaveBeenCalledWith('open_pr_detail_window', expect.anything()); }); it('clicking the title also opens the pop-out', () => { diff --git a/src/BorgDock.Tauri/src/components/pr/__tests__/PrCardContainer.test.tsx b/src/BorgDock.Tauri/src/components/pr/__tests__/PrCardContainer.test.tsx index 0336c0275..d40db9038 100644 --- a/src/BorgDock.Tauri/src/components/pr/__tests__/PrCardContainer.test.tsx +++ b/src/BorgDock.Tauri/src/components/pr/__tests__/PrCardContainer.test.tsx @@ -1,6 +1,6 @@ import { cleanup, fireEvent, render, screen } from '@testing-library/react'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; -import type { PullRequestWithChecks } from '@/types'; +import type { CheckRun, PullRequestWithChecks } from '@/types'; import { PrCardContainer } from '../PrCardContainer'; afterEach(cleanup); @@ -63,7 +63,9 @@ vi.mock('@/stores/ui-store', () => { }); vi.mock('@/stores/pr-store', () => { - const fn = vi.fn() as unknown as ReturnType & { getState: ReturnType }; + const fn = vi.fn() as unknown as ReturnType & { + getState: ReturnType; + }; fn.mockImplementation((selector: (state: Record) => unknown) => { return selector({ username: 'testuser' }); }); @@ -96,7 +98,20 @@ vi.mock('@/components/pr-detail/LinkedWorkItemBadge', () => ({ ), })); -function makePr(overrides: Partial = {}): PullRequestWithChecks { +function makePr( + overrides: Partial & { checks?: CheckRun[] } = {}, +): PullRequestWithChecks { + const { checks: checksOverride, ...rest } = overrides; + const checks = checksOverride ?? [ + { + id: 1, + name: 'build', + status: 'completed', + conclusion: 'success', + htmlUrl: '', + checkSuiteId: 1, + }, + ]; return { pullRequest: { number: 42, @@ -122,23 +137,19 @@ function makePr(overrides: Partial = {}): PullRequestWith changedFiles: 5, commitCount: 3, requestedReviewers: [], - ...overrides.pullRequest, + ...rest.pullRequest, }, - checks: overrides.checks ?? [ - { - id: 1, - name: 'build', - status: 'completed', - conclusion: 'success', - htmlUrl: '', - checkSuiteId: 1, - }, - ], - overallStatus: overrides.overallStatus ?? 'green', - failedCheckNames: overrides.failedCheckNames ?? [], - pendingCheckNames: overrides.pendingCheckNames ?? [], - passedCount: overrides.passedCount ?? 1, - skippedCount: overrides.skippedCount ?? 0, + overallStatus: rest.overallStatus ?? 'green', + failedCheckNames: rest.failedCheckNames ?? [], + failedCheckSuiteIds: + rest.failedCheckSuiteIds ?? + checks + .filter((c: CheckRun) => c.conclusion === 'failure' || c.conclusion === 'timed_out') + .map((c: CheckRun) => c.checkSuiteId), + pendingCheckNames: rest.pendingCheckNames ?? [], + passedCount: rest.passedCount ?? 1, + skippedCount: rest.skippedCount ?? 0, + totalCheckCount: rest.totalCheckCount ?? checks.length, }; } @@ -376,9 +387,7 @@ describe('PrCardContainer', () => { })} />, ); - expect( - container.querySelector('[data-pr-primary-action="rerun"]'), - ).toBeInTheDocument(); + expect(container.querySelector('[data-pr-primary-action="rerun"]')).toBeInTheDocument(); }); it('chooses "merge" as primary when PR is approved and owned by current user', () => { @@ -394,9 +403,7 @@ describe('PrCardContainer', () => { })} />, ); - expect( - container.querySelector('[data-pr-primary-action="merge"]'), - ).toBeInTheDocument(); + expect(container.querySelector('[data-pr-primary-action="merge"]')).toBeInTheDocument(); }); it('chooses "checkout" as primary when PR is owned but not yet approved', () => { @@ -410,9 +417,7 @@ describe('PrCardContainer', () => { })} />, ); - expect( - container.querySelector('[data-pr-primary-action="checkout"]'), - ).toBeInTheDocument(); + expect(container.querySelector('[data-pr-primary-action="checkout"]')).toBeInTheDocument(); }); it('renders Checkout secondary button when primary is not checkout', () => { diff --git a/src/BorgDock.Tauri/src/components/pr/__tests__/PrContextMenu.test.tsx b/src/BorgDock.Tauri/src/components/pr/__tests__/PrContextMenu.test.tsx index 2aaf7ec95..9a8b04c4f 100644 --- a/src/BorgDock.Tauri/src/components/pr/__tests__/PrContextMenu.test.tsx +++ b/src/BorgDock.Tauri/src/components/pr/__tests__/PrContextMenu.test.tsx @@ -1,6 +1,6 @@ import { cleanup, fireEvent, render, screen } from '@testing-library/react'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; -import type { PullRequestWithChecks } from '@/types'; +import type { CheckRun, PullRequestWithChecks } from '@/types'; import { PrContextMenu } from '../PrContextMenu'; afterEach(cleanup); @@ -64,7 +64,11 @@ vi.mock('@/services/github/mutations', () => ({ mergePullRequest: vi.fn().mockResolvedValue(undefined), })); -function makePr(overrides: Partial = {}): PullRequestWithChecks { +function makePr( + overrides: Partial & { checks?: CheckRun[] } = {}, +): PullRequestWithChecks { + const { checks: checksOverride, ...rest } = overrides; + const checks = checksOverride ?? []; return { pullRequest: { number: 42, @@ -89,14 +93,19 @@ function makePr(overrides: Partial = {}): PullRequestWith changedFiles: 2, commitCount: 1, requestedReviewers: [], - ...overrides.pullRequest, + ...rest.pullRequest, }, - checks: overrides.checks ?? [], - overallStatus: overrides.overallStatus ?? 'green', - failedCheckNames: overrides.failedCheckNames ?? [], - pendingCheckNames: overrides.pendingCheckNames ?? [], - passedCount: overrides.passedCount ?? 0, - skippedCount: overrides.skippedCount ?? 0, + overallStatus: rest.overallStatus ?? 'green', + failedCheckNames: rest.failedCheckNames ?? [], + failedCheckSuiteIds: + rest.failedCheckSuiteIds ?? + checks + .filter((c: CheckRun) => c.conclusion === 'failure' || c.conclusion === 'timed_out') + .map((c: CheckRun) => c.checkSuiteId), + pendingCheckNames: rest.pendingCheckNames ?? [], + passedCount: rest.passedCount ?? 0, + skippedCount: rest.skippedCount ?? 0, + totalCheckCount: rest.totalCheckCount ?? checks.length, }; } diff --git a/src/BorgDock.Tauri/src/components/pr/__tests__/PrList.test.tsx b/src/BorgDock.Tauri/src/components/pr/__tests__/PrList.test.tsx index 86407e5ab..9c19f1629 100644 --- a/src/BorgDock.Tauri/src/components/pr/__tests__/PrList.test.tsx +++ b/src/BorgDock.Tauri/src/components/pr/__tests__/PrList.test.tsx @@ -58,7 +58,9 @@ vi.mock('@/services/github/singleton', () => ({ let mockStoreState: Record = {}; vi.mock('@/stores/pr-store', () => { - const fn = vi.fn() as unknown as ReturnType & { getState: ReturnType }; + const fn = vi.fn() as unknown as ReturnType & { + getState: ReturnType; + }; fn.mockImplementation((selector: (state: Record) => unknown) => { return selector(mockStoreState); }); @@ -95,12 +97,13 @@ function makePr(number: number, repo = 'test/repo'): PullRequestWithChecks { commitCount: 1, requestedReviewers: [], }, - checks: [], overallStatus: 'green', failedCheckNames: [], + failedCheckSuiteIds: [], pendingCheckNames: [], passedCount: 0, skippedCount: 0, + totalCheckCount: 0, }; } diff --git a/src/BorgDock.Tauri/src/components/pr/__tests__/RepoGroup.test.tsx b/src/BorgDock.Tauri/src/components/pr/__tests__/RepoGroup.test.tsx index f4017605b..be75eb111 100644 --- a/src/BorgDock.Tauri/src/components/pr/__tests__/RepoGroup.test.tsx +++ b/src/BorgDock.Tauri/src/components/pr/__tests__/RepoGroup.test.tsx @@ -32,7 +32,9 @@ vi.mock('@/stores/ui-store', () => { }); vi.mock('@/stores/pr-store', () => { - const fn = vi.fn() as unknown as ReturnType & { getState: ReturnType }; + const fn = vi.fn() as unknown as ReturnType & { + getState: ReturnType; + }; fn.mockImplementation((selector: (state: Record) => unknown) => { return selector({ username: 'testuser' }); }); @@ -93,12 +95,13 @@ function makePr( commitCount: 1, requestedReviewers: [], }, - checks: [], overallStatus: status, failedCheckNames: [], + failedCheckSuiteIds: [], pendingCheckNames: [], passedCount: 0, skippedCount: 0, + totalCheckCount: 0, }; } diff --git a/src/BorgDock.Tauri/src/components/settings/GitHubSection.tsx b/src/BorgDock.Tauri/src/components/settings/GitHubSection.tsx index 2c7467698..94a14f7cd 100644 --- a/src/BorgDock.Tauri/src/components/settings/GitHubSection.tsx +++ b/src/BorgDock.Tauri/src/components/settings/GitHubSection.tsx @@ -1,17 +1,44 @@ import { invoke } from '@tauri-apps/api/core'; import { openUrl } from '@tauri-apps/plugin-opener'; -import { Card, Button, LinearProgress } from '@/components/shared/primitives'; -import { Field, SectionHeader, Seg2, Slider, TextInput } from '@/components/shared/primitives'; +import { + Button, + Card, + Field, + LinearProgress, + SectionHeader, + Seg2, + Slider, + TextInput, +} from '@/components/shared/primitives'; import { useGitHubRateLimit } from '@/services/github/rate-limit'; import type { GitHubSettings } from '@/types/settings'; -interface Props { github: GitHubSettings; onChange: (g: GitHubSettings) => void } +interface Props { + github: GitHubSettings; + onChange: (g: GitHubSettings) => void; +} -export function GitHubSection({ github, onChange }: Props) { - const rl = useGitHubRateLimit(); - const pct = rl ? (rl.used / rl.limit) * 100 : 0; +function RateLimitRow({ label, pool }: { label: string; pool?: { used: number; limit: number } }) { + const pct = pool ? (pool.used / pool.limit) * 100 : 0; const tone: 'success' | 'warning' | 'error' = pct >= 95 ? 'error' : pct >= 80 ? 'warning' : 'success'; + return ( +
+ + {label} + +
+ +
+ + {pool ? `${pool.used.toLocaleString()} / ${pool.limit.toLocaleString()}` : '—'} + +
+ ); +} + +export function GitHubSection({ github, onChange }: Props) { + const rl = useGitHubRateLimit(); return ( <> @@ -20,7 +47,9 @@ export function GitHubSection({ github, onChange }: Props) { subtitle="How BorgDock authenticates with github.com and how often it polls for new pull requests." /> -

Authentication

+

+ Authentication +

{github.username || '—'} -
@@ -85,22 +118,35 @@ export function GitHubSection({ github, onChange }: Props) { /> - -
-
- -
- - {rl ? `${rl.used.toLocaleString()} / ${rl.limit.toLocaleString()}` : '—'} - + +
+ +
- -
diff --git a/src/BorgDock.Tauri/src/components/settings/__tests__/GitHubSection.test.tsx b/src/BorgDock.Tauri/src/components/settings/__tests__/GitHubSection.test.tsx index 617dd3bff..95257e5b9 100644 --- a/src/BorgDock.Tauri/src/components/settings/__tests__/GitHubSection.test.tsx +++ b/src/BorgDock.Tauri/src/components/settings/__tests__/GitHubSection.test.tsx @@ -3,8 +3,9 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import type { GitHubSettings } from '@/types'; import { GitHubSection } from '../GitHubSection'; +const mockRateLimit = vi.fn<() => unknown>(() => null); vi.mock('@/services/github/rate-limit', () => ({ - useGitHubRateLimit: () => null, + useGitHubRateLimit: () => mockRateLimit(), })); vi.mock('@tauri-apps/plugin-opener', () => ({ @@ -98,6 +99,20 @@ describe('GitHubSection', () => { expect(screen.getByRole('button', { name: 'Test connection' })).toBeDefined(); }); + it('renders both REST and GraphQL rate-limit rows', () => { + mockRateLimit.mockReturnValue({ + rest: { used: 120, limit: 5000, resetAt: 0 }, + graphql: { used: 45, limit: 5000, resetAt: 0 }, + }); + render(); + + expect(screen.getByText('REST')).toBeDefined(); + expect(screen.getByText('GraphQL')).toBeDefined(); + expect(screen.getByText('120 / 5,000')).toBeDefined(); + expect(screen.getByText('45 / 5,000')).toBeDefined(); + mockRateLimit.mockReturnValue(null); + }); + it('preserves existing settings when updating a single field', () => { const github = makeGitHub({ authMethod: 'pat', personalAccessToken: 'ghp_xxx' }); render(); diff --git a/src/BorgDock.Tauri/src/hooks/__tests__/useClaudeActions.test.ts b/src/BorgDock.Tauri/src/hooks/__tests__/useClaudeActions.test.ts index c9ff07f0d..49aedaecc 100644 --- a/src/BorgDock.Tauri/src/hooks/__tests__/useClaudeActions.test.ts +++ b/src/BorgDock.Tauri/src/hooks/__tests__/useClaudeActions.test.ts @@ -73,12 +73,13 @@ function makePr( commitCount: 1, requestedReviewers: [], }, - checks: [], overallStatus: 'red', failedCheckNames: ['build'], + failedCheckSuiteIds: [], pendingCheckNames: [], passedCount: 0, skippedCount: 0, + totalCheckCount: 1, }; } diff --git a/src/BorgDock.Tauri/src/hooks/__tests__/useExternalMergeCelebration.test.ts b/src/BorgDock.Tauri/src/hooks/__tests__/useExternalMergeCelebration.test.ts index 5270f131f..f9f045655 100644 --- a/src/BorgDock.Tauri/src/hooks/__tests__/useExternalMergeCelebration.test.ts +++ b/src/BorgDock.Tauri/src/hooks/__tests__/useExternalMergeCelebration.test.ts @@ -28,7 +28,9 @@ const mockGitHub = { username: 'alice' }; vi.mock('@/stores/settings-store', () => ({ useSettingsStore: { - getState: () => ({ settings: { notifications: mockSettings.notifications, gitHub: mockGitHub } }), + getState: () => ({ + settings: { notifications: mockSettings.notifications, gitHub: mockGitHub }, + }), }, })); @@ -62,12 +64,13 @@ function makePr(overrides: Partial = {}): PullRequestWithChecks { requestedReviewers: [], ...overrides, }, - checks: [], overallStatus: 'gray', failedCheckNames: [], + failedCheckSuiteIds: [], pendingCheckNames: [], passedCount: 0, skippedCount: 0, + totalCheckCount: 0, }; } @@ -101,9 +104,7 @@ describe('useExternalMergeCelebration', () => { act(() => { usePrStore.setState({ pullRequests: [], - closedPullRequests: [ - makePr({ state: 'closed', mergedAt: '2026-04-30T10:00:00Z' }), - ], + closedPullRequests: [makePr({ state: 'closed', mergedAt: '2026-04-30T10:00:00Z' })], }); }); @@ -120,9 +121,7 @@ describe('useExternalMergeCelebration', () => { act(() => { usePrStore.setState({ pullRequests: [], - closedPullRequests: [ - makePr({ state: 'closed', mergedAt: '2026-04-30T10:00:00Z' }), - ], + closedPullRequests: [makePr({ state: 'closed', mergedAt: '2026-04-30T10:00:00Z' })], }); }); @@ -140,9 +139,7 @@ describe('useExternalMergeCelebration', () => { act(() => { usePrStore.setState({ pullRequests: [], - closedPullRequests: [ - makePr({ state: 'closed', closedAt: '2026-04-30T10:00:00Z' }), - ], + closedPullRequests: [makePr({ state: 'closed', closedAt: '2026-04-30T10:00:00Z' })], }); }); @@ -158,9 +155,7 @@ describe('useExternalMergeCelebration', () => { act(() => { usePrStore.setState({ pullRequests: [], - closedPullRequests: [ - makePr({ state: 'closed', mergedAt: '2026-04-30T10:00:00Z' }), - ], + closedPullRequests: [makePr({ state: 'closed', mergedAt: '2026-04-30T10:00:00Z' })], }); }); @@ -212,9 +207,7 @@ describe('useExternalMergeCelebration', () => { act(() => { usePrStore.setState({ pullRequests: [], - closedPullRequests: [ - makePr({ state: 'closed', mergedAt: '2026-04-30T10:00:00Z' }), - ], + closedPullRequests: [makePr({ state: 'closed', mergedAt: '2026-04-30T10:00:00Z' })], }); }); expect(mockCelebrate).toHaveBeenCalledTimes(1); @@ -228,9 +221,7 @@ describe('useExternalMergeCelebration', () => { act(() => { usePrStore.setState({ pullRequests: [], - closedPullRequests: [ - makePr({ state: 'closed', mergedAt: '2026-04-30T10:00:00Z' }), - ], + closedPullRequests: [makePr({ state: 'closed', mergedAt: '2026-04-30T10:00:00Z' })], }); }); diff --git a/src/BorgDock.Tauri/src/hooks/__tests__/useGitHubPolling.test.ts b/src/BorgDock.Tauri/src/hooks/__tests__/useGitHubPolling.test.ts index fa25ffd29..a89dad854 100644 --- a/src/BorgDock.Tauri/src/hooks/__tests__/useGitHubPolling.test.ts +++ b/src/BorgDock.Tauri/src/hooks/__tests__/useGitHubPolling.test.ts @@ -1,13 +1,12 @@ import { act, renderHook } from '@testing-library/react'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; -import type { AppSettings, PullRequest } from '@/types'; +import type { AppSettings, PullRequest, PullRequestWithChecks } from '@/types'; // --- Mock all external dependencies --- const mockGetGitHubToken = vi.fn(); -const mockGetOpenPRs = vi.fn(); +const mockPollOpenPrsAggregate = vi.fn(); const mockGetClosedPRs = vi.fn(); -const mockGetCheckRunsForRef = vi.fn(); const mockAggregatePrWithChecks = vi.fn(); const mockInitClient = vi.fn(); const mockGetClient = vi.fn(); @@ -16,13 +15,12 @@ vi.mock('@/services/github/auth', () => ({ getGitHubToken: (...args: unknown[]) => mockGetGitHubToken(...args), })); -vi.mock('@/services/github/pulls', () => ({ - getOpenPRs: (...args: unknown[]) => mockGetOpenPRs(...args), - getClosedPRs: (...args: unknown[]) => mockGetClosedPRs(...args), +vi.mock('@/services/github/polling', () => ({ + pollOpenPrsAggregate: (...args: unknown[]) => mockPollOpenPrsAggregate(...args), })); -vi.mock('@/services/github/checks', () => ({ - getCheckRunsForRef: (...args: unknown[]) => mockGetCheckRunsForRef(...args), +vi.mock('@/services/github/pulls', () => ({ + getClosedPRs: (...args: unknown[]) => mockGetClosedPRs(...args), })); vi.mock('@/services/github/aggregate', () => ({ @@ -36,6 +34,7 @@ const mockClientInstance = { hadFreshData: true, isRateLimitLow: false, getRateLimit: vi.fn().mockReturnValue({ remaining: 5000, total: 5000, reset: new Date() }), + getGraphqlRateLimit: vi.fn().mockReturnValue({ remaining: 5000, total: 5000, reset: new Date() }), getEtagEntries: vi.fn().mockReturnValue([]), }; @@ -94,7 +93,14 @@ function makeSettings(overrides: Partial = {}): AppSettings { channels: { tray: true, system: true, sound: true, emailDigest: false }, }, claudeCode: { defaultPostFixAction: 'none' }, - claudeApi: { model: 'claude-sonnet-4-20250514', maxTokens: 4096, prSummaryEnabled: true, diffExplanationsEnabled: true, reviewNudgePhrasingEnabled: false, commitMessageSuggestionsEnabled: false }, + claudeApi: { + model: 'claude-sonnet-4-20250514', + maxTokens: 4096, + prSummaryEnabled: true, + diffExplanationsEnabled: true, + reviewNudgePhrasingEnabled: false, + commitMessageSuggestionsEnabled: false, + }, claudeReview: { botUsername: '' }, updates: { autoCheckEnabled: true, autoDownload: false }, azureDevOps: { @@ -146,6 +152,20 @@ function makePr(overrides: Partial = {}): PullRequest { }; } +/** Fully-aggregated shape, as pollOpenPrsAggregate returns it. */ +function makePrw(overrides: Partial = {}): PullRequestWithChecks { + return { + pullRequest: makePr(overrides), + overallStatus: 'green', + failedCheckNames: [], + failedCheckSuiteIds: [], + pendingCheckNames: [], + passedCount: 1, + skippedCount: 0, + totalCheckCount: 1, + }; +} + describe('useGitHubPolling', () => { let originalFetch: typeof globalThis.fetch; @@ -166,17 +186,17 @@ describe('useGitHubPolling', () => { // Default mock behaviors mockGetGitHubToken.mockResolvedValue('ghp_test123'); mockGetClient.mockReturnValue(mockClientInstance); - mockGetOpenPRs.mockResolvedValue([]); + mockPollOpenPrsAggregate.mockResolvedValue([]); mockGetClosedPRs.mockResolvedValue([]); - mockGetCheckRunsForRef.mockResolvedValue([]); mockAggregatePrWithChecks.mockImplementation((pr: PullRequest, checks: unknown[]) => ({ pullRequest: pr, - checks, overallStatus: 'green', failedCheckNames: [], + failedCheckSuiteIds: [], pendingCheckNames: [], passedCount: 0, skippedCount: 0, + totalCheckCount: checks.length, })); mockClientInstance.hadFreshData = true; mockClientInstance.isRateLimitLow = false; @@ -185,6 +205,11 @@ describe('useGitHubPolling', () => { total: 5000, reset: new Date(), }); + mockClientInstance.getGraphqlRateLimit.mockReturnValue({ + remaining: 5000, + total: 5000, + reset: new Date(), + }); // Mock global fetch for username detection originalFetch = globalThis.fetch; @@ -215,13 +240,13 @@ describe('useGitHubPolling', () => { }); expect(mockInitClient).not.toHaveBeenCalled(); - expect(mockGetOpenPRs).not.toHaveBeenCalled(); + expect(mockPollOpenPrsAggregate).not.toHaveBeenCalled(); expect(mockFetchFn).not.toHaveBeenCalled(); expect(usePrStore.getState().isPolling).toBe(false); }); it('starts polling immediately when enabled flips from false to true', async () => { - mockGetOpenPRs.mockResolvedValue([makePr()]); + mockPollOpenPrsAggregate.mockResolvedValue([makePrw()]); const { rerender } = renderHook( ({ enabled }: { enabled: boolean }) => useGitHubPolling(makeSettings(), enabled), @@ -239,7 +264,7 @@ describe('useGitHubPolling', () => { await vi.waitFor(() => { expect(mockInitClient).toHaveBeenCalledTimes(1); - expect(mockGetOpenPRs).toHaveBeenCalledWith(mockClientInstance, 'test', 'repo'); + expect(mockPollOpenPrsAggregate).toHaveBeenCalledWith(mockClientInstance, 'test', 'repo'); }); }); }); @@ -299,9 +324,8 @@ describe('useGitHubPolling', () => { expect(typeof result.current.pollNow).toBe('function'); }); - it('starts polling and fetches open PRs', async () => { - const pr = makePr(); - mockGetOpenPRs.mockResolvedValue([pr]); + it('starts polling and fetches open PRs via one aggregate call per repo', async () => { + mockPollOpenPrsAggregate.mockResolvedValue([makePrw()]); renderHook(() => useGitHubPolling(makeSettings())); @@ -311,51 +335,12 @@ describe('useGitHubPolling', () => { }); await vi.waitFor(() => { - expect(mockGetOpenPRs).toHaveBeenCalledWith(mockClientInstance, 'test', 'repo'); - }); - }); - - it('fetches check runs for each PR', async () => { - const pr = makePr({ headRef: 'my-branch' }); - mockGetOpenPRs.mockResolvedValue([pr]); - mockGetCheckRunsForRef.mockResolvedValue([{ id: 1, name: 'build', status: 'completed' }]); - - renderHook(() => useGitHubPolling(makeSettings())); - - await act(async () => { - vi.advanceTimersByTime(0); - }); - - await vi.waitFor(() => { - expect(mockGetCheckRunsForRef).toHaveBeenCalledWith( - mockClientInstance, - 'test', - 'repo', - 'my-branch', - ); - }); - }); - - it('aggregates PR with checks results', async () => { - const pr = makePr(); - const checks = [{ id: 1, name: 'build' }]; - mockGetOpenPRs.mockResolvedValue([pr]); - mockGetCheckRunsForRef.mockResolvedValue(checks); - - renderHook(() => useGitHubPolling(makeSettings())); - - await act(async () => { - vi.advanceTimersByTime(0); - }); - - await vi.waitFor(() => { - expect(mockAggregatePrWithChecks).toHaveBeenCalledWith(pr, checks); + expect(mockPollOpenPrsAggregate).toHaveBeenCalledWith(mockClientInstance, 'test', 'repo'); }); }); it('updates PR store with results when data is fresh', async () => { - const pr = makePr(); - mockGetOpenPRs.mockResolvedValue([pr]); + mockPollOpenPrsAggregate.mockResolvedValue([makePrw()]); mockClientInstance.hadFreshData = true; renderHook(() => useGitHubPolling(makeSettings())); @@ -370,8 +355,7 @@ describe('useGitHubPolling', () => { }); it('still updates PR store even when data is not fresh (304 — onResult always fires)', async () => { - const pr = makePr(); - mockGetOpenPRs.mockResolvedValue([pr]); + mockPollOpenPrsAggregate.mockResolvedValue([makePrw()]); mockClientInstance.hadFreshData = false; usePrStore.getState().setPullRequests([]); @@ -383,7 +367,7 @@ describe('useGitHubPolling', () => { }); await vi.waitFor(() => { - expect(mockGetOpenPRs).toHaveBeenCalled(); + expect(mockPollOpenPrsAggregate).toHaveBeenCalled(); }); // onResult always fires regardless of hadFreshData — the ETag cache @@ -393,13 +377,18 @@ describe('useGitHubPolling', () => { }); }); - it('sets rate limit in store after successful poll', async () => { - mockGetOpenPRs.mockResolvedValue([]); + it('sets rate limit in store from the tighter of the two pools', async () => { + mockPollOpenPrsAggregate.mockResolvedValue([]); mockClientInstance.getRateLimit.mockReturnValue({ remaining: 4500, total: 5000, reset: new Date('2024-01-01T01:00:00Z'), }); + mockClientInstance.getGraphqlRateLimit.mockReturnValue({ + remaining: 4900, + total: 5000, + reset: new Date('2024-01-01T01:00:00Z'), + }); renderHook(() => useGitHubPolling(makeSettings())); @@ -415,13 +404,44 @@ describe('useGitHubPolling', () => { }); }); - it('skips rate limit update when remaining is negative', async () => { - mockGetOpenPRs.mockResolvedValue([]); + it('uses the GraphQL pool when the REST pool has no data yet', async () => { + mockPollOpenPrsAggregate.mockResolvedValue([]); + mockClientInstance.getRateLimit.mockReturnValue({ + remaining: -1, + total: -1, + reset: null, + }); + mockClientInstance.getGraphqlRateLimit.mockReturnValue({ + remaining: 4800, + total: 5000, + reset: new Date('2024-01-01T01:00:00Z'), + }); + + renderHook(() => useGitHubPolling(makeSettings())); + + await act(async () => { + vi.advanceTimersByTime(0); + }); + + await vi.waitFor(() => { + const rl = usePrStore.getState().rateLimit; + expect(rl).toBeDefined(); + expect(rl!.remaining).toBe(4800); + }); + }); + + it('skips rate limit update when both pools are unknown', async () => { + mockPollOpenPrsAggregate.mockResolvedValue([]); mockClientInstance.getRateLimit.mockReturnValue({ remaining: -1, total: -1, reset: null, }); + mockClientInstance.getGraphqlRateLimit.mockReturnValue({ + remaining: -1, + total: -1, + reset: null, + }); renderHook(() => useGitHubPolling(makeSettings())); @@ -430,16 +450,18 @@ describe('useGitHubPolling', () => { }); await vi.waitFor(() => { - expect(mockGetOpenPRs).toHaveBeenCalled(); + expect(mockPollOpenPrsAggregate).toHaveBeenCalled(); }); expect(usePrStore.getState().rateLimit).toBeNull(); }); - it('handles getCheckRunsForRef failure gracefully (no prior state)', async () => { - const pr = makePr(); - mockGetOpenPRs.mockResolvedValue([pr]); - mockGetCheckRunsForRef.mockRejectedValue(new Error('check fetch failed')); + it('keeps last-known PRs for a repo when its poll fails', async () => { + // Seed the store as if the previous cycle had succeeded. + const prior = makePrw({ number: 7, title: 'Survivor' }); + usePrStore.setState({ pullRequests: [prior] }); + + mockPollOpenPrsAggregate.mockRejectedValue(new Error('graphql boom')); renderHook(() => useGitHubPolling(makeSettings())); @@ -448,51 +470,51 @@ describe('useGitHubPolling', () => { }); await vi.waitFor(() => { - // With no prior state for this PR, fall back to empty checks. - expect(mockAggregatePrWithChecks).toHaveBeenCalledWith(pr, []); + expect(mockPollOpenPrsAggregate).toHaveBeenCalled(); }); - }); - it('reuses prior checks when getCheckRunsForRef fails (avoids spurious gray→green transition)', async () => { - const pr = makePr(); - const priorChecks = [ - { - id: 42, - name: 'build', - status: 'completed', - conclusion: 'success', - htmlUrl: '', - checkSuiteId: 1, - }, - ]; + // The failed repo's PRs survive the cycle instead of vanishing (which + // would fire spurious "new PR" notifications when they reappear). + await vi.waitFor(() => { + const prs = usePrStore.getState().pullRequests; + expect(prs).toHaveLength(1); + expect(prs[0]).toBe(prior); + }); + }); - // Seed the store with a previous aggregation for this PR, as if the last - // successful poll had reported green. - usePrStore.setState({ - pullRequests: [ - { - pullRequest: pr, - checks: priorChecks, - overallStatus: 'green', - failedCheckNames: [], - pendingCheckNames: [], - passedCount: 1, - skippedCount: 0, - }, + it("does not resurrect another repo's PRs when one repo fails", async () => { + const settings = makeSettings({ + repos: [ + { owner: 'o1', name: 'r1', enabled: true, worktreeBasePath: '', worktreeSubfolder: '' }, + { owner: 'o2', name: 'r2', enabled: true, worktreeBasePath: '', worktreeSubfolder: '' }, ], }); - mockGetOpenPRs.mockResolvedValue([pr]); - mockGetCheckRunsForRef.mockRejectedValue(new Error('check fetch failed')); + const r1Prior = makePrw({ number: 1, repoOwner: 'o1', repoName: 'r1' }); + const r2Prior = makePrw({ number: 2, repoOwner: 'o2', repoName: 'r2' }); + usePrStore.setState({ pullRequests: [r1Prior, r2Prior] }); - renderHook(() => useGitHubPolling(makeSettings())); + const r2Fresh = makePrw({ number: 2, repoOwner: 'o2', repoName: 'r2', title: 'fresh' }); + mockPollOpenPrsAggregate.mockImplementation((_c: unknown, owner: string) => { + if (owner === 'o1') return Promise.reject(new Error('boom')); + return Promise.resolve([r2Fresh]); + }); + + renderHook(() => useGitHubPolling(settings)); await act(async () => { vi.advanceTimersByTime(0); }); + // Second repo runs after the 500ms stagger. + await act(async () => { + vi.advanceTimersByTime(500); + }); await vi.waitFor(() => { - expect(mockAggregatePrWithChecks).toHaveBeenCalledWith(pr, priorChecks); + const prs = usePrStore.getState().pullRequests; + expect(prs).toHaveLength(2); + expect(prs).toContain(r1Prior); // failed repo: last-known entry kept + expect(prs).toContain(r2Fresh); // healthy repo: fresh result used }); }); @@ -516,7 +538,7 @@ describe('useGitHubPolling', () => { ], }); - mockGetOpenPRs.mockResolvedValue([]); + mockPollOpenPrsAggregate.mockResolvedValue([]); renderHook(() => useGitHubPolling(settings)); @@ -525,8 +547,12 @@ describe('useGitHubPolling', () => { }); await vi.waitFor(() => { - expect(mockGetOpenPRs).toHaveBeenCalledWith(mockClientInstance, 'test', 'repo1'); - expect(mockGetOpenPRs).not.toHaveBeenCalledWith(mockClientInstance, 'test', 'repo2'); + expect(mockPollOpenPrsAggregate).toHaveBeenCalledWith(mockClientInstance, 'test', 'repo1'); + expect(mockPollOpenPrsAggregate).not.toHaveBeenCalledWith( + mockClientInstance, + 'test', + 'repo2', + ); }); }); @@ -553,11 +579,11 @@ describe('useGitHubPolling', () => { expect(mockGetClient).toHaveBeenCalled(); }); - expect(mockGetOpenPRs).not.toHaveBeenCalled(); + expect(mockPollOpenPrsAggregate).not.toHaveBeenCalled(); }); - it('handles getOpenPRs failure for a repo gracefully', async () => { - mockGetOpenPRs.mockRejectedValue(new Error('API error')); + it('completes the poll cycle when a repo fetch fails', async () => { + mockPollOpenPrsAggregate.mockRejectedValue(new Error('API error')); renderHook(() => useGitHubPolling(makeSettings())); @@ -568,7 +594,11 @@ describe('useGitHubPolling', () => { // The poll cycle should complete despite the error (structured logger // captures it; the error doesn't propagate to the caller). await vi.waitFor(() => { - expect(mockGetOpenPRs).toHaveBeenCalled(); + expect(mockPollOpenPrsAggregate).toHaveBeenCalled(); + }); + + await vi.waitFor(() => { + expect(usePrStore.getState().lastPollTime).not.toBeNull(); }); }); @@ -601,24 +631,24 @@ describe('useGitHubPolling', () => { }); it('stops polling on unmount', async () => { - mockGetOpenPRs.mockResolvedValue([]); + mockPollOpenPrsAggregate.mockResolvedValue([]); const { unmount } = renderHook(() => useGitHubPolling(makeSettings())); unmount(); - const callCountAfterUnmount = mockGetOpenPRs.mock.calls.length; + const callCountAfterUnmount = mockPollOpenPrsAggregate.mock.calls.length; // Advance timers significantly — should not trigger more polls await act(async () => { vi.advanceTimersByTime(300_000); }); - expect(mockGetOpenPRs.mock.calls.length).toBe(callCountAfterUnmount); + expect(mockPollOpenPrsAggregate.mock.calls.length).toBe(callCountAfterUnmount); }); it('pollNow triggers an immediate poll', async () => { - mockGetOpenPRs.mockResolvedValue([]); + mockPollOpenPrsAggregate.mockResolvedValue([]); const { result } = renderHook(() => useGitHubPolling(makeSettings())); @@ -628,20 +658,20 @@ describe('useGitHubPolling', () => { }); await vi.waitFor(() => { - expect(mockGetOpenPRs).toHaveBeenCalled(); + expect(mockPollOpenPrsAggregate).toHaveBeenCalled(); }); - const initialCallCount = mockGetOpenPRs.mock.calls.length; + const initialCallCount = mockPollOpenPrsAggregate.mock.calls.length; await act(async () => { await result.current.pollNow(); }); - expect(mockGetOpenPRs.mock.calls.length).toBeGreaterThan(initialCallCount); + expect(mockPollOpenPrsAggregate.mock.calls.length).toBeGreaterThan(initialCallCount); }); it('pollNow sets polling state', async () => { - mockGetOpenPRs.mockResolvedValue([]); + mockPollOpenPrsAggregate.mockResolvedValue([]); const { result } = renderHook(() => useGitHubPolling(makeSettings())); @@ -650,7 +680,7 @@ describe('useGitHubPolling', () => { }); await vi.waitFor(() => { - expect(mockGetOpenPRs).toHaveBeenCalled(); + expect(mockPollOpenPrsAggregate).toHaveBeenCalled(); }); // pollNow should set isPolling to true @@ -738,7 +768,7 @@ describe('useGitHubPolling', () => { ], }); - mockGetOpenPRs.mockResolvedValue([]); + mockPollOpenPrsAggregate.mockResolvedValue([]); renderHook(() => useGitHubPolling(settings)); @@ -748,7 +778,7 @@ describe('useGitHubPolling', () => { }); await vi.waitFor(() => { - expect(mockGetOpenPRs).toHaveBeenCalledWith(mockClientInstance, 'o1', 'r1'); + expect(mockPollOpenPrsAggregate).toHaveBeenCalledWith(mockClientInstance, 'o1', 'r1'); }); // Second repo is called after 500ms stagger @@ -757,12 +787,12 @@ describe('useGitHubPolling', () => { }); await vi.waitFor(() => { - expect(mockGetOpenPRs).toHaveBeenCalledWith(mockClientInstance, 'o2', 'r2'); + expect(mockPollOpenPrsAggregate).toHaveBeenCalledWith(mockClientInstance, 'o2', 'r2'); }); }); it('marks poll start on client before fetching', async () => { - mockGetOpenPRs.mockResolvedValue([]); + mockPollOpenPrsAggregate.mockResolvedValue([]); renderHook(() => useGitHubPolling(makeSettings())); diff --git a/src/BorgDock.Tauri/src/hooks/__tests__/useInitSequence.test.tsx b/src/BorgDock.Tauri/src/hooks/__tests__/useInitSequence.test.tsx index e7dd4b2ba..19c24a9f2 100644 --- a/src/BorgDock.Tauri/src/hooks/__tests__/useInitSequence.test.tsx +++ b/src/BorgDock.Tauri/src/hooks/__tests__/useInitSequence.test.tsx @@ -62,7 +62,14 @@ function makeSettings(overrides: Partial = {}): AppSettings { channels: { tray: true, system: true, sound: true, emailDigest: false }, }, claudeCode: { defaultPostFixAction: 'none' }, - claudeApi: { model: 'claude-sonnet-4-20250514', maxTokens: 4096, prSummaryEnabled: true, diffExplanationsEnabled: true, reviewNudgePhrasingEnabled: false, commitMessageSuggestionsEnabled: false }, + claudeApi: { + model: 'claude-sonnet-4-20250514', + maxTokens: 4096, + prSummaryEnabled: true, + diffExplanationsEnabled: true, + reviewNudgePhrasingEnabled: false, + commitMessageSuggestionsEnabled: false, + }, claudeReview: { botUsername: '' }, updates: { autoCheckEnabled: true, autoDownload: false }, azureDevOps: { @@ -141,12 +148,13 @@ describe('useInitSequence', () => { mockGetOpenPRs.mockResolvedValue([makePr()]); mockAggregatePrWithChecks.mockImplementation((pr: PullRequest) => ({ pullRequest: pr, - checks: [], overallStatus: 'gray', failedCheckNames: [], + failedCheckSuiteIds: [], pendingCheckNames: [], passedCount: 0, skippedCount: 0, + totalCheckCount: 0, })); vi.stubGlobal( diff --git a/src/BorgDock.Tauri/src/hooks/__tests__/useKeyboardNav.test.ts b/src/BorgDock.Tauri/src/hooks/__tests__/useKeyboardNav.test.ts index 9a5b37888..5c97b9e04 100644 --- a/src/BorgDock.Tauri/src/hooks/__tests__/useKeyboardNav.test.ts +++ b/src/BorgDock.Tauri/src/hooks/__tests__/useKeyboardNav.test.ts @@ -87,12 +87,13 @@ function makePr(number: number): PullRequestWithChecks { commitCount: 1, requestedReviewers: [], } as PullRequest, - checks: [], overallStatus: 'green', failedCheckNames: [], + failedCheckSuiteIds: [], pendingCheckNames: [], passedCount: 1, skippedCount: 0, + totalCheckCount: 1, }; } diff --git a/src/BorgDock.Tauri/src/hooks/__tests__/usePrCardActions.test.ts b/src/BorgDock.Tauri/src/hooks/__tests__/usePrCardActions.test.ts index 9d516ebf0..5353dd857 100644 --- a/src/BorgDock.Tauri/src/hooks/__tests__/usePrCardActions.test.ts +++ b/src/BorgDock.Tauri/src/hooks/__tests__/usePrCardActions.test.ts @@ -83,17 +83,21 @@ function makePr(overrides: Partial = {}): PullRequest { function makePrWithChecks(overrides: Partial = {}): PullRequestWithChecks { return { pullRequest: makePr(), - checks: [], overallStatus: 'green', failedCheckNames: [], + failedCheckSuiteIds: [], pendingCheckNames: [], passedCount: 0, skippedCount: 0, + totalCheckCount: 0, ...overrides, }; } -const fakeMouseEvent = { stopPropagation: () => {}, preventDefault: () => {} } as unknown as React.MouseEvent; +const fakeMouseEvent = { + stopPropagation: () => {}, + preventDefault: () => {}, +} as unknown as React.MouseEvent; // --- Tests --- @@ -119,7 +123,12 @@ describe('usePrCardActions', () => { await Promise.resolve(); }); expect(mockMergePr).toHaveBeenCalledWith( - expect.objectContaining({ number: 42, repoOwner: 'owner', repoName: 'repo', title: 'Add feature X' }), + expect.objectContaining({ + number: 42, + repoOwner: 'owner', + repoName: 'repo', + title: 'Add feature X', + }), ); }); diff --git a/src/BorgDock.Tauri/src/hooks/__tests__/useQuickReviewKeyboard.test.ts b/src/BorgDock.Tauri/src/hooks/__tests__/useQuickReviewKeyboard.test.ts index 039d7046b..ffa2c2d3c 100644 --- a/src/BorgDock.Tauri/src/hooks/__tests__/useQuickReviewKeyboard.test.ts +++ b/src/BorgDock.Tauri/src/hooks/__tests__/useQuickReviewKeyboard.test.ts @@ -64,12 +64,13 @@ function makePr(): PullRequestWithChecks { commitCount: 1, requestedReviewers: [], }, - checks: [], overallStatus: 'green', failedCheckNames: [], + failedCheckSuiteIds: [], pendingCheckNames: [], passedCount: 1, skippedCount: 0, + totalCheckCount: 1, }; } diff --git a/src/BorgDock.Tauri/src/hooks/__tests__/useReviewNudges.test.ts b/src/BorgDock.Tauri/src/hooks/__tests__/useReviewNudges.test.ts index 4a0d2947c..4a42ac35a 100644 --- a/src/BorgDock.Tauri/src/hooks/__tests__/useReviewNudges.test.ts +++ b/src/BorgDock.Tauri/src/hooks/__tests__/useReviewNudges.test.ts @@ -31,8 +31,7 @@ vi.mock('@/services/notification', async () => { await vi.importActual('@/services/notification'); return { ...actual, - buildReviewNudgeNotification: (...args: unknown[]) => - mockBuildReviewNudgeNotification(...args), + buildReviewNudgeNotification: (...args: unknown[]) => mockBuildReviewNudgeNotification(...args), sendOsNotification: mockSendOs, }; }); @@ -70,12 +69,13 @@ function makePr(number: number): PullRequestWithChecks { commitCount: 1, requestedReviewers: ['testuser'], }, - checks: [], overallStatus: 'green', failedCheckNames: [], + failedCheckSuiteIds: [], pendingCheckNames: [], passedCount: 1, skippedCount: 0, + totalCheckCount: 1, }; } @@ -119,7 +119,14 @@ function makeSettings( channels: { tray: true, system: true, sound: true, emailDigest: false }, }, claudeCode: { defaultPostFixAction: 'none' }, - claudeApi: { model: 'claude-sonnet-4-20250514', maxTokens: 4096, prSummaryEnabled: true, diffExplanationsEnabled: true, reviewNudgePhrasingEnabled: false, commitMessageSuggestionsEnabled: false }, + claudeApi: { + model: 'claude-sonnet-4-20250514', + maxTokens: 4096, + prSummaryEnabled: true, + diffExplanationsEnabled: true, + reviewNudgePhrasingEnabled: false, + commitMessageSuggestionsEnabled: false, + }, claudeReview: { botUsername: '' }, updates: { autoCheckEnabled: true, autoDownload: false }, azureDevOps: { diff --git a/src/BorgDock.Tauri/src/hooks/__tests__/useStateTransitions.test.ts b/src/BorgDock.Tauri/src/hooks/__tests__/useStateTransitions.test.ts index aba2870df..58f0cf903 100644 --- a/src/BorgDock.Tauri/src/hooks/__tests__/useStateTransitions.test.ts +++ b/src/BorgDock.Tauri/src/hooks/__tests__/useStateTransitions.test.ts @@ -59,12 +59,13 @@ function makePrWithChecks( const { pr, ...rest } = overrides; return { pullRequest: makePr(pr), - checks: [], overallStatus: 'gray', failedCheckNames: [], + failedCheckSuiteIds: [], pendingCheckNames: [], passedCount: 0, skippedCount: 0, + totalCheckCount: 0, ...rest, }; } @@ -104,7 +105,14 @@ function makeSettings(overrides: Partial = {}): AppSettings { ...overrides.notifications, }, claudeCode: { defaultPostFixAction: 'none' }, - claudeApi: { model: 'claude-sonnet-4-20250514', maxTokens: 4096, prSummaryEnabled: true, diffExplanationsEnabled: true, reviewNudgePhrasingEnabled: false, commitMessageSuggestionsEnabled: false }, + claudeApi: { + model: 'claude-sonnet-4-20250514', + maxTokens: 4096, + prSummaryEnabled: true, + diffExplanationsEnabled: true, + reviewNudgePhrasingEnabled: false, + commitMessageSuggestionsEnabled: false, + }, claudeReview: { botUsername: '' }, updates: { autoCheckEnabled: true, autoDownload: false }, azureDevOps: { diff --git a/src/BorgDock.Tauri/src/hooks/useFlyoutSync.ts b/src/BorgDock.Tauri/src/hooks/useFlyoutSync.ts index c6f1b449e..8beb9813d 100644 --- a/src/BorgDock.Tauri/src/hooks/useFlyoutSync.ts +++ b/src/BorgDock.Tauri/src/hooks/useFlyoutSync.ts @@ -1,16 +1,11 @@ import { useEffect, useRef } from 'react'; import { useClaudeActions } from '@/hooks/useClaudeActions'; import { computeMergeScore } from '@/services/merge-score'; -import type { PrActionId } from '@/services/pr-action-resolver'; -import { - checkoutPrBranch, - mergePr, - openPrInBrowser, - rerunChecks, -} from '@/services/pr-actions'; -import { usePrStore } from '@/stores/pr-store'; import { sendOsNotification } from '@/services/notification'; +import type { PrActionId } from '@/services/pr-action-resolver'; +import { checkoutPrBranch, mergePr, openPrInBrowser, rerunChecks } from '@/services/pr-actions'; import { openPrDetail } from '@/services/windows'; +import { usePrStore } from '@/stores/pr-store'; import { useSettingsStore } from '@/stores/settings-store'; import type { PullRequestWithChecks } from '@/types'; @@ -64,7 +59,7 @@ function buildFlyoutPayload( failedCheckNames: pr.failedCheckNames, pendingCount: pr.pendingCheckNames.length, passedCount: pr.passedCount, - totalChecks: pr.checks.length, + totalChecks: pr.totalCheckCount, commentCount: pr.pullRequest.commentCount, isMine: lowerUser ? pr.pullRequest.authorLogin.toLowerCase() === lowerUser : false, // Included so the flyout's context menu can copy locally without @@ -441,14 +436,12 @@ export function useFlyoutSync() { // identically regardless of which surface fired the event. switch (action) { case 'rerun': { - const failedCheck = prw.checks.find( - (c) => c.conclusion === 'failure' || c.conclusion === 'timed_out', - ); - if (failedCheck) { + const failedSuiteId = prw.failedCheckSuiteIds[0]; + if (failedSuiteId !== undefined) { void rerunChecks({ repoOwner: pr.repoOwner, repoName: pr.repoName, - checkSuiteId: failedCheck.checkSuiteId, + checkSuiteId: failedSuiteId, }); } break; diff --git a/src/BorgDock.Tauri/src/hooks/useGitHubPolling.ts b/src/BorgDock.Tauri/src/hooks/useGitHubPolling.ts index 81db07761..a8951ee62 100644 --- a/src/BorgDock.Tauri/src/hooks/useGitHubPolling.ts +++ b/src/BorgDock.Tauri/src/hooks/useGitHubPolling.ts @@ -2,8 +2,8 @@ import { useCallback, useEffect, useRef } from 'react'; import { saveCachedEtags, saveCachedPRs } from '@/services/cache'; import { aggregatePrWithChecks } from '@/services/github/aggregate'; import { getGitHubToken } from '@/services/github/auth'; -import { getCheckRunsForRef } from '@/services/github/checks'; -import { getClosedPRs, getOpenPRs } from '@/services/github/pulls'; +import { pollOpenPrsAggregate } from '@/services/github/polling'; +import { getClosedPRs } from '@/services/github/pulls'; import { getClient, initClient } from '@/services/github/singleton'; import { createLogger } from '@/services/logger'; import { PollingManager } from '@/services/polling'; @@ -66,10 +66,10 @@ export function useGitHubPolling(settings: AppSettings, enabled: boolean = true) return []; } - // Snapshot prior state so a transient check-runs fetch failure can fall - // back to the PR's last-known checks instead of resetting overallStatus - // to 'gray'. Otherwise the next successful poll looks like a gray→green - // transition and fires a spurious "All checks passed" notification. + // Snapshot prior state so a transient repo-level fetch failure keeps the + // repo's last-known PRs in the list instead of dropping them for a cycle. + // Otherwise they "reappear" on the next successful poll and fire spurious + // new-PR / status-transition notifications. const priorByKey = new Map(); for (const prior of usePrStore.getState().pullRequests) { const p = prior.pullRequest; @@ -89,50 +89,31 @@ export function useGitHubPolling(settings: AppSettings, enabled: boolean = true) const repoLabel = `${repo.owner}/${repo.name}`; try { const repoStart = performance.now(); - const prs = await getOpenPRs(c, repo.owner, repo.name); - - // Fetch check runs for all PRs in parallel - const results = await Promise.allSettled( - prs.map(async (pr) => { - const checks = await getCheckRunsForRef(c, repo.owner, repo.name, pr.headRef); - return aggregatePrWithChecks(pr, checks); - }), - ); - - let failedCheckFetches = 0; - for (let j = 0; j < results.length; j++) { - const result = results[j]!; - if (result.status === 'fulfilled') { - allPrs.push(result.value); - } else { - failedCheckFetches++; - const failedPr = prs[j]!; - log.warn('poll: check-runs fetch failed for PR', { - repo: repoLabel, - pr: failedPr.number, - ref: failedPr.headRef, - error: String(result.reason), - }); - const prior = priorByKey.get(`${repo.owner}/${repo.name}#${failedPr.number}`); - allPrs.push(aggregatePrWithChecks(failedPr, prior?.checks ?? [])); - } + const prs = await pollOpenPrsAggregate(c, repo.owner, repo.name); + for (const pr of prs) { + allPrs.push(pr); } log.debug('poll: repo fetched', { repo: repoLabel, prs: prs.length, - failedCheckFetches, durationMs: Math.round(performance.now() - repoStart), }); } catch (err) { - log.error('poll: repo failed', err, { repo: repoLabel }); + log.error('poll: repo failed — keeping last-known PRs', err, { repo: repoLabel }); + for (const prior of priorByKey.values()) { + const p = prior.pullRequest; + if (p.repoOwner === repo.owner && p.repoName === repo.name) { + allPrs.push(prior); + } + } } } log.info('poll cycle done', { totalPrs: allPrs.length, durationMs: Math.round(performance.now() - pollStart), - rateLimitRemaining: c.getRateLimit().remaining, + rateLimitRemaining: c.getGraphqlRateLimit().remaining, }); return allPrs; @@ -147,7 +128,14 @@ export function useGitHubPolling(settings: AppSettings, enabled: boolean = true) usePrStore.getState().setPullRequests(results); usePrStore.getState().setPollingState(false, new Date()); - const rl = client.getRateLimit(); + // REST and GraphQL are separate pools — surface whichever is tighter + // (polling spends GraphQL points; cold paths still spend REST). + const rest = client.getRateLimit(); + const graphql = client.getGraphqlRateLimit(); + const rl = + rest.remaining >= 0 && (graphql.remaining < 0 || rest.remaining < graphql.remaining) + ? rest + : graphql; if (rl.remaining >= 0) { usePrStore.getState().setRateLimit({ remaining: rl.remaining, diff --git a/src/BorgDock.Tauri/src/hooks/usePrCardActions.ts b/src/BorgDock.Tauri/src/hooks/usePrCardActions.ts index 314f4ef29..ed71c9438 100644 --- a/src/BorgDock.Tauri/src/hooks/usePrCardActions.ts +++ b/src/BorgDock.Tauri/src/hooks/usePrCardActions.ts @@ -23,7 +23,7 @@ import { parseError } from '@/utils/parse-error'; * (confirmAction, contextMenu) and the React event glue. */ export function usePrCardActions(prWithChecks: PullRequestWithChecks) { - const { pullRequest: pr, checks, failedCheckNames } = prWithChecks; + const { pullRequest: pr, failedCheckNames } = prWithChecks; const { fixWithClaude, monitorPr, resolveConflicts } = useClaudeActions(); const settings = useSettingsStore((s) => s.settings); @@ -33,9 +33,8 @@ export function usePrCardActions(prWithChecks: PullRequestWithChecks) { const repoConfig = findRepoConfig(settings.repos, pr.repoOwner, pr.repoName); const repoPath = repoConfig?.worktreeBasePath || ''; - const failedCheck = checks.find( - (c) => c.conclusion === 'failure' || c.conclusion === 'timed_out', - ); + // First failed check's suite — what the rerun action targets. + const failedSuiteId: number | undefined = prWithChecks.failedCheckSuiteIds[0]; const showError = useCallback((title: string, err: unknown) => { void sendOsNotification({ @@ -53,14 +52,14 @@ export function usePrCardActions(prWithChecks: PullRequestWithChecks) { const handleRerun = useCallback( (e: React.MouseEvent) => { e.stopPropagation(); - if (!failedCheck) return; + if (failedSuiteId === undefined) return; void rerunChecks({ repoOwner: pr.repoOwner, repoName: pr.repoName, - checkSuiteId: failedCheck.checkSuiteId, + checkSuiteId: failedSuiteId, }); }, - [failedCheck, pr.repoOwner, pr.repoName], + [failedSuiteId, pr.repoOwner, pr.repoName], ); const handleFix = useCallback( @@ -216,7 +215,7 @@ export function usePrCardActions(prWithChecks: PullRequestWithChecks) { confirmAction, setConfirmAction, repoPath, - failedCheck, + failedSuiteId, // Handlers handleContextMenu, diff --git a/src/BorgDock.Tauri/src/services/__tests__/cache.test.ts b/src/BorgDock.Tauri/src/services/__tests__/cache.test.ts index 385a67c8f..aecd29b4e 100644 --- a/src/BorgDock.Tauri/src/services/__tests__/cache.test.ts +++ b/src/BorgDock.Tauri/src/services/__tests__/cache.test.ts @@ -129,19 +129,82 @@ describe('cache service', () => { }); describe('loadCachedPRs', () => { - it('returns cached PR data', async () => { - const prs = [{ pullRequest: { number: 1 } }]; - mockInvoke.mockResolvedValue(prs); + it('passes current-shape entries through untouched', async () => { + const entry = { + pullRequest: { number: 1 }, + overallStatus: 'green', + failedCheckNames: [], + failedCheckSuiteIds: [], + pendingCheckNames: [], + passedCount: 1, + skippedCount: 0, + totalCheckCount: 1, + }; + mockInvoke.mockResolvedValue([entry]); const result = await loadCachedPRs('owner', 'repo'); - expect(result).toEqual(prs); + expect(result[0]).toBe(entry); expect(mockInvoke).toHaveBeenCalledWith('cache_load_prs', { repoOwner: 'owner', repoName: 'repo', }); }); + it('migrates pre-GraphQL entries by re-deriving summary fields from legacy checks', async () => { + // Shape written by versions that stored the raw checks array and had no + // totalCheckCount / failedCheckSuiteIds. Reading failedCheckSuiteIds[0] + // off such an entry crashed PrCardContainer at boot. + const legacy = { + pullRequest: { number: 7 }, + checks: [ + { + id: 1, + name: 'build', + status: 'completed', + conclusion: 'failure', + htmlUrl: '', + checkSuiteId: 11, + }, + { + id: 2, + name: 'test', + status: 'completed', + conclusion: 'success', + htmlUrl: '', + checkSuiteId: 22, + }, + ], + overallStatus: 'red', + failedCheckNames: ['build'], + pendingCheckNames: [], + passedCount: 1, + skippedCount: 0, + }; + mockInvoke.mockResolvedValue([legacy]); + + const [pr] = (await loadCachedPRs('owner', 'repo')) as Array>; + + expect(pr!.pullRequest).toEqual({ number: 7 }); + expect(pr!.totalCheckCount).toBe(2); + expect(pr!.failedCheckSuiteIds).toEqual([11]); + expect(pr!.failedCheckNames).toEqual(['build']); + expect(pr!.overallStatus).toBe('red'); + expect(pr!.passedCount).toBe(1); + expect(pr!.checks).toBeUndefined(); + }); + + it('migrates legacy entries without a checks array to zero counts', async () => { + const legacy = { pullRequest: { number: 3 }, overallStatus: 'gray' }; + mockInvoke.mockResolvedValue([legacy]); + + const [pr] = (await loadCachedPRs('owner', 'repo')) as Array>; + + expect(pr!.totalCheckCount).toBe(0); + expect(pr!.failedCheckSuiteIds).toEqual([]); + expect(pr!.overallStatus).toBe('gray'); + }); + it('returns empty array on error', async () => { mockInvoke.mockRejectedValue(new Error('fail')); diff --git a/src/BorgDock.Tauri/src/services/__tests__/claude-launcher.test.ts b/src/BorgDock.Tauri/src/services/__tests__/claude-launcher.test.ts index 92903578f..4c0b511a2 100644 --- a/src/BorgDock.Tauri/src/services/__tests__/claude-launcher.test.ts +++ b/src/BorgDock.Tauri/src/services/__tests__/claude-launcher.test.ts @@ -28,12 +28,13 @@ function makePrWithChecks(overrides: Partial = {}): PullR commitCount: 1, requestedReviewers: [], }, - checks: [], overallStatus: 'red', failedCheckNames: ['build'], + failedCheckSuiteIds: [], pendingCheckNames: [], passedCount: 0, skippedCount: 0, + totalCheckCount: 1, ...overrides, }; } diff --git a/src/BorgDock.Tauri/src/services/__tests__/notification.test.ts b/src/BorgDock.Tauri/src/services/__tests__/notification.test.ts index c9a36bea5..1f57c6f66 100644 --- a/src/BorgDock.Tauri/src/services/__tests__/notification.test.ts +++ b/src/BorgDock.Tauri/src/services/__tests__/notification.test.ts @@ -47,12 +47,13 @@ function makePrWithChecks( const { pr, ...rest } = overrides; return { pullRequest: makePr(pr), - checks: [], overallStatus: 'gray', failedCheckNames: [], + failedCheckSuiteIds: [], pendingCheckNames: [], passedCount: 0, skippedCount: 0, + totalCheckCount: 0, ...rest, }; } diff --git a/src/BorgDock.Tauri/src/services/__tests__/priority-scoring.test.ts b/src/BorgDock.Tauri/src/services/__tests__/priority-scoring.test.ts index 7b378776d..a264f89f9 100644 --- a/src/BorgDock.Tauri/src/services/__tests__/priority-scoring.test.ts +++ b/src/BorgDock.Tauri/src/services/__tests__/priority-scoring.test.ts @@ -41,12 +41,13 @@ function makePrWithChecks( ): PullRequestWithChecks { return { pullRequest: makePr(prOverrides), - checks: [], overallStatus: 'green', failedCheckNames: [], + failedCheckSuiteIds: [], pendingCheckNames: [], passedCount: 1, skippedCount: 0, + totalCheckCount: 1, ...overrides, }; } diff --git a/src/BorgDock.Tauri/src/services/__tests__/team-review-load.test.ts b/src/BorgDock.Tauri/src/services/__tests__/team-review-load.test.ts index 5bc2786d4..43185f623 100644 --- a/src/BorgDock.Tauri/src/services/__tests__/team-review-load.test.ts +++ b/src/BorgDock.Tauri/src/services/__tests__/team-review-load.test.ts @@ -40,12 +40,13 @@ function makePrWithChecks( ): PullRequestWithChecks { return { pullRequest: makePr(prOverrides), - checks: [], overallStatus: 'green', failedCheckNames: [], + failedCheckSuiteIds: [], pendingCheckNames: [], passedCount: 1, skippedCount: 0, + totalCheckCount: 1, ...overrides, }; } diff --git a/src/BorgDock.Tauri/src/services/cache.ts b/src/BorgDock.Tauri/src/services/cache.ts index 8ee763c5a..c29330048 100644 --- a/src/BorgDock.Tauri/src/services/cache.ts +++ b/src/BorgDock.Tauri/src/services/cache.ts @@ -1,5 +1,7 @@ import { invoke } from '@tauri-apps/api/core'; +import { aggregatePrWithChecks } from '@/services/github/aggregate'; import { createLogger } from '@/services/logger'; +import type { CheckRun, PullRequest, PullRequestWithChecks } from '@/types'; const log = createLogger('cache'); @@ -100,9 +102,27 @@ export async function saveCachedEtags(entries: CachedEtagEntry[]): Promise // PR list cache (wires up existing but unused commands) // --------------------------------------------------------------------------- +/** + * Cached PR entries written before the GraphQL polling migration carry a raw + * `checks` array and lack `totalCheckCount` / `failedCheckSuiteIds`. Reading + * such an entry crashes consumers (`failedCheckSuiteIds[0]`), so re-derive the + * current shape from the legacy checks via the same aggregator that produced + * the old entry. Current-shape entries pass through untouched. + */ +function normalizeCachedPr(raw: unknown): unknown { + if (!raw || typeof raw !== 'object') return raw; + const entry = raw as Partial & { checks?: CheckRun[] }; + if (!entry.pullRequest) return raw; + if (entry.totalCheckCount !== undefined && entry.failedCheckSuiteIds !== undefined) { + return raw; + } + return aggregatePrWithChecks(entry.pullRequest as PullRequest, entry.checks ?? []); +} + export async function loadCachedPRs(repoOwner: string, repoName: string): Promise { try { - return await invoke('cache_load_prs', { repoOwner, repoName }); + const prs = await invoke('cache_load_prs', { repoOwner, repoName }); + return prs.map(normalizeCachedPr); } catch (err) { log.warn('loadCachedPRs failed (returning empty)', { repoOwner, repoName, error: String(err) }); return []; diff --git a/src/BorgDock.Tauri/src/services/github/__tests__/aggregate.test.ts b/src/BorgDock.Tauri/src/services/github/__tests__/aggregate.test.ts index 6d8076b7f..a759d02e8 100644 --- a/src/BorgDock.Tauri/src/services/github/__tests__/aggregate.test.ts +++ b/src/BorgDock.Tauri/src/services/github/__tests__/aggregate.test.ts @@ -165,14 +165,13 @@ describe('aggregatePrWithChecks', () => { expect(result.skippedCount).toBe(2); }); - it('includes the PR and checks in the result', () => { + it('includes the PR in the result', () => { const pr = makePullRequest({ number: 99, title: 'My PR' }); const checks = [makeCheckRun({ name: 'build', conclusion: 'success' })]; const result = aggregatePrWithChecks(pr, checks); expect(result.pullRequest).toBe(pr); - expect(result.checks).toBe(checks); }); it('handles empty checks array', () => { @@ -182,8 +181,48 @@ describe('aggregatePrWithChecks', () => { expect(result.overallStatus).toBe('gray'); expect(result.failedCheckNames).toEqual([]); + expect(result.failedCheckSuiteIds).toEqual([]); expect(result.pendingCheckNames).toEqual([]); expect(result.passedCount).toBe(0); expect(result.skippedCount).toBe(0); + expect(result.totalCheckCount).toBe(0); + }); + + it('returns failedCheckSuiteIds parallel to failedCheckNames', () => { + const pr = makePullRequest(); + const checks = [ + makeCheckRun({ name: 'build', conclusion: 'failure', checkSuiteId: 11 }), + makeCheckRun({ id: 2, name: 'test', conclusion: 'success', checkSuiteId: 22 }), + makeCheckRun({ id: 3, name: 'deploy', conclusion: 'timed_out', checkSuiteId: 33 }), + ]; + + const result = aggregatePrWithChecks(pr, checks); + + expect(result.failedCheckNames).toEqual(['build', 'deploy']); + expect(result.failedCheckSuiteIds).toEqual([11, 33]); + }); + + it('drops failedCheckSuiteIds entries with non-positive suite id', () => { + const pr = makePullRequest(); + const checks = [ + makeCheckRun({ name: 'orphan', conclusion: 'failure', checkSuiteId: 0 }), + makeCheckRun({ id: 2, name: 'real', conclusion: 'failure', checkSuiteId: 99 }), + ]; + + const result = aggregatePrWithChecks(pr, checks); + + expect(result.failedCheckNames).toEqual(['orphan', 'real']); + expect(result.failedCheckSuiteIds).toEqual([99]); + }); + + it('returns totalCheckCount equal to the input length', () => { + const pr = makePullRequest(); + const checks = [ + makeCheckRun({ name: 'a', conclusion: 'success' }), + makeCheckRun({ id: 2, name: 'b', conclusion: 'failure' }), + makeCheckRun({ id: 3, name: 'c', conclusion: 'skipped' }), + ]; + + expect(aggregatePrWithChecks(pr, checks).totalCheckCount).toBe(3); }); }); diff --git a/src/BorgDock.Tauri/src/services/github/__tests__/client.test.ts b/src/BorgDock.Tauri/src/services/github/__tests__/client.test.ts index 30e21119e..06b7d942f 100644 --- a/src/BorgDock.Tauri/src/services/github/__tests__/client.test.ts +++ b/src/BorgDock.Tauri/src/services/github/__tests__/client.test.ts @@ -1,5 +1,20 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; -import { GitHubApiError, GitHubAuthError, GitHubClient } from '../client'; +import { GitHubApiError, GitHubAuthError, GitHubClient, GitHubRateLimitError } from '../client'; + +// Spy on the structured logger so graphql start/ok/failed lines can be asserted. +const logSpies = vi.hoisted(() => ({ + debug: vi.fn(), + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), +})); +vi.mock('@/services/logger', () => ({ + createLogger: () => ({ + ...logSpies, + time: async (_label: string, fn: () => Promise) => fn(), + child: () => ({ ...logSpies }), + }), +})); function createClient(token = 'test-token') { return new GitHubClient(vi.fn().mockResolvedValue(token)); @@ -373,6 +388,122 @@ describe('GitHubClient', () => { expect(body.query).toBe('query { viewer { login } }'); expect(body.variables).toBeUndefined(); }); + + it('throws GitHubAuthError on 401', async () => { + const client = createClient(); + fetchSpy.mockResolvedValueOnce(mockResponse(401)); + + await expect(client.graphql('query { viewer { login } }')).rejects.toThrow(GitHubAuthError); + }); + + it('throws GitHubRateLimitError on 403 when the GraphQL pool is exhausted', async () => { + const client = createClient(); + fetchSpy.mockResolvedValueOnce( + mockResponse(403, {}, { 'X-RateLimit-Remaining': '0', 'X-RateLimit-Reset': '1750000000' }), + ); + + await expect(client.graphql('query { viewer { login } }')).rejects.toThrow( + GitHubRateLimitError, + ); + }); + + it('logs a named graphql ok line per successful call', async () => { + logSpies.info.mockClear(); + const client = createClient(); + fetchSpy.mockResolvedValueOnce(mockResponse(200, { data: { ok: true } })); + + await client.graphql('query PollOpenPrs($owner: String!) { __typename }'); + + const lines = logSpies.info.mock.calls.map((c) => JSON.stringify(c)); + expect(lines.some((l) => l.includes('graphql start') && l.includes('PollOpenPrs'))).toBe( + true, + ); + expect(lines.some((l) => l.includes('graphql ok') && l.includes('PollOpenPrs'))).toBe(true); + }); + }); + + describe('rate-limit pools', () => { + it('REST get does not affect the GraphQL pool', async () => { + const client = createClient(); + fetchSpy.mockResolvedValueOnce( + mockResponse(200, [], { 'X-RateLimit-Remaining': '4000', 'X-RateLimit-Limit': '5000' }), + ); + + await client.get('repos/x/y/pulls?state=open'); + + expect(client.getRateLimit().remaining).toBe(4000); + expect(client.getGraphqlRateLimit().remaining).toBe(-1); + }); + + it('graphql call updates the GraphQL pool from response headers, not the REST pool', async () => { + const client = createClient(); + fetchSpy.mockResolvedValueOnce( + mockResponse( + 200, + { data: { ok: true } }, + { 'X-RateLimit-Remaining': '4900', 'X-RateLimit-Limit': '5000' }, + ), + ); + + await client.graphql('query { __typename }'); + + expect(client.getRateLimit().remaining).toBe(-1); + expect(client.getGraphqlRateLimit().remaining).toBe(4900); + expect(client.getGraphqlRateLimit().total).toBe(5000); + }); + + it('inline body rateLimit wins over headers for the GraphQL pool', async () => { + const client = createClient(); + fetchSpy.mockResolvedValueOnce( + mockResponse( + 200, + { + data: { + repository: { id: '1' }, + rateLimit: { remaining: 4500, limit: 5000, resetAt: '2026-05-07T12:00:00Z', cost: 3 }, + }, + }, + { 'X-RateLimit-Remaining': '4998' }, + ), + ); + + await client.graphql('query PollOpenPrs { __typename }'); + + const rl = client.getGraphqlRateLimit(); + expect(rl.remaining).toBe(4500); + expect(rl.total).toBe(5000); + expect(rl.reset?.toISOString()).toBe('2026-05-07T12:00:00.000Z'); + expect(client.getRateLimit().remaining).toBe(-1); + }); + + it('isRateLimitLow is true when only the GraphQL pool is low', async () => { + const client = createClient(); + fetchSpy.mockResolvedValueOnce( + mockResponse(200, { + data: { rateLimit: { remaining: 100, limit: 5000, resetAt: '2026-05-07T12:00:00Z' } }, + }), + ); + + await client.graphql('query { __typename }'); + + expect(client.getRateLimit().remaining).toBe(-1); + expect(client.isRateLimitLow).toBe(true); + }); + + it('isRateLimitLow is false when both pools are healthy', async () => { + const client = createClient(); + fetchSpy.mockResolvedValueOnce(mockResponse(200, [], { 'X-RateLimit-Remaining': '4000' })); + fetchSpy.mockResolvedValueOnce( + mockResponse(200, { + data: { rateLimit: { remaining: 4999, limit: 5000, resetAt: '2026-05-07T12:00:00Z' } }, + }), + ); + + await client.get('repos/x/y'); + await client.graphql('query { __typename }'); + + expect(client.isRateLimitLow).toBe(false); + }); }); describe('fetchWithRetry', () => { diff --git a/src/BorgDock.Tauri/src/services/github/__tests__/polling.test.ts b/src/BorgDock.Tauri/src/services/github/__tests__/polling.test.ts new file mode 100644 index 000000000..6366e55e7 --- /dev/null +++ b/src/BorgDock.Tauri/src/services/github/__tests__/polling.test.ts @@ -0,0 +1,443 @@ +import { describe, expect, it, vi } from 'vitest'; +import type { PullRequestWithChecks } from '@/types'; +import { GitHubClient } from '../client'; +import { POLL_OPEN_PRS_QUERY, pollOpenPrsAggregate } from '../polling'; + +function makeNode(overrides: Record = {}) { + return { + number: 1, + title: 'Test PR', + body: '', + url: 'https://github.com/octocat/hello-world/pull/1', + state: 'OPEN', + isDraft: false, + mergeable: 'MERGEABLE', + createdAt: '2026-05-01T00:00:00Z', + updatedAt: '2026-05-01T00:00:00Z', + closedAt: null, + mergedAt: null, + additions: 10, + deletions: 5, + changedFiles: 2, + author: { login: 'octocat', avatarUrl: 'https://x/y.png' }, + baseRefName: 'main', + headRefName: 'feature', + headRefOid: 'sha1', + labels: { nodes: [{ name: 'bug' }] }, + reviewRequests: { nodes: [] }, + commits: { totalCount: 1, nodes: [{ commit: { statusCheckRollup: null } }] }, + latestReviews: { nodes: [] }, + comments: { totalCount: 3 }, + reviewThreads: { totalCount: 2 }, + ...overrides, + }; +} + +function makeRollup(contexts: Array>, state = 'SUCCESS') { + return { + commits: { + totalCount: 1, + nodes: [ + { + commit: { + statusCheckRollup: { + state, + contexts: { pageInfo: { hasNextPage: false }, nodes: contexts }, + }, + }, + }, + ], + }, + }; +} + +function makeClient(nodes: unknown[], totalCount = nodes.length): GitHubClient { + const client = new GitHubClient(vi.fn().mockResolvedValue('t')); + vi.spyOn(client, 'graphql').mockResolvedValue({ + repository: { pullRequests: { totalCount, nodes } }, + }); + return client; +} + +function callAdapter(nodes: unknown[]): Promise { + return pollOpenPrsAggregate(makeClient(nodes), 'octocat', 'hello-world'); +} + +describe('POLL_OPEN_PRS_QUERY', () => { + it('is a non-empty string', () => { + expect(typeof POLL_OPEN_PRS_QUERY).toBe('string'); + expect(POLL_OPEN_PRS_QUERY.length).toBeGreaterThan(0); + }); + + it('declares owner and repo variables', () => { + expect(POLL_OPEN_PRS_QUERY).toContain('$owner: String!'); + expect(POLL_OPEN_PRS_QUERY).toContain('$repo: String!'); + }); + + it('requests rateLimit at the query root (outside repository)', () => { + expect(POLL_OPEN_PRS_QUERY).toContain('rateLimit'); + const repoClose = POLL_OPEN_PRS_QUERY.lastIndexOf('rateLimit'); + const repoOpen = POLL_OPEN_PRS_QUERY.indexOf('repository('); + expect(repoOpen).toBeGreaterThanOrEqual(0); + expect(repoClose).toBeGreaterThan(repoOpen); + }); + + it('requests the status check rollup with pagination info', () => { + expect(POLL_OPEN_PRS_QUERY).toContain('statusCheckRollup'); + expect(POLL_OPEN_PRS_QUERY).toContain('hasNextPage'); + }); +}); + +describe('pollOpenPrsAggregate', () => { + it('passes owner and repo as GraphQL variables', async () => { + const client = makeClient([]); + await pollOpenPrsAggregate(client, 'octocat', 'hello-world'); + + expect(client.graphql).toHaveBeenCalledWith(POLL_OPEN_PRS_QUERY, { + owner: 'octocat', + repo: 'hello-world', + }); + }); + + it('returns empty array when repo has no open PRs', async () => { + const result = await callAdapter([]); + expect(result).toEqual([]); + }); + + it('returns empty array when repository is null (inaccessible)', async () => { + const client = new GitHubClient(vi.fn().mockResolvedValue('t')); + vi.spyOn(client, 'graphql').mockResolvedValue({ repository: null }); + + const result = await pollOpenPrsAggregate(client, 'octocat', 'gone'); + expect(result).toEqual([]); + }); + + it('returns empty array when the response data is undefined (malformed 200)', async () => { + const client = new GitHubClient(vi.fn().mockResolvedValue('t')); + vi.spyOn(client, 'graphql').mockResolvedValue(undefined); + + const result = await pollOpenPrsAggregate(client, 'octocat', 'hello-world'); + expect(result).toEqual([]); + }); + + it('returns empty array when the response body is not an object (stubbed network)', async () => { + // e2e harnesses answer api.github.com with `[]` — `[].repository` must not throw. + const client = new GitHubClient(vi.fn().mockResolvedValue('t')); + vi.spyOn(client, 'graphql').mockResolvedValue([]); + + const result = await pollOpenPrsAggregate(client, 'octocat', 'hello-world'); + expect(result).toEqual([]); + }); + + it('returns empty array when repository.pullRequests is missing', async () => { + const client = new GitHubClient(vi.fn().mockResolvedValue('t')); + vi.spyOn(client, 'graphql').mockResolvedValue({ repository: {} }); + + const result = await pollOpenPrsAggregate(client, 'octocat', 'hello-world'); + expect(result).toEqual([]); + }); + + it('caps the result at 100 PRs and does not throw when more exist', async () => { + const nodes = Array.from({ length: 100 }, (_, i) => makeNode({ number: i + 1 })); + const client = makeClient(nodes, 250); + + const result = await pollOpenPrsAggregate(client, 'big', 'repo'); + expect(result.length).toBe(100); + }); +}); + +describe('pollOpenPrsAggregate (adapter)', () => { + it('maps a basic PR node with no rollup → overallStatus gray', async () => { + const [pr] = await callAdapter([makeNode()]); + + expect(pr).toBeDefined(); + expect(pr!.pullRequest.number).toBe(1); + expect(pr!.pullRequest.title).toBe('Test PR'); + expect(pr!.pullRequest.state).toBe('open'); + expect(pr!.pullRequest.repoOwner).toBe('octocat'); + expect(pr!.pullRequest.repoName).toBe('hello-world'); + expect(pr!.pullRequest.headRef).toBe('feature'); + expect(pr!.pullRequest.headSha).toBe('sha1'); + expect(pr!.pullRequest.baseRef).toBe('main'); + expect(pr!.pullRequest.authorLogin).toBe('octocat'); + expect(pr!.pullRequest.authorAvatarUrl).toBe('https://x/y.png'); + expect(pr!.pullRequest.commentCount).toBe(5); // 3 comments + 2 review threads + expect(pr!.pullRequest.commitCount).toBe(1); + expect(pr!.pullRequest.labels).toEqual(['bug']); + expect(pr!.pullRequest.mergeable).toBe(true); + expect(pr!.overallStatus).toBe('gray'); + expect(pr!.totalCheckCount).toBe(0); + expect(pr!.failedCheckNames).toEqual([]); + expect(pr!.failedCheckSuiteIds).toEqual([]); + }); + + it('mergeable: CONFLICTING → false', async () => { + const [pr] = await callAdapter([makeNode({ mergeable: 'CONFLICTING' })]); + expect(pr!.pullRequest.mergeable).toBe(false); + }); + + it('mergeable: UNKNOWN → undefined', async () => { + const [pr] = await callAdapter([makeNode({ mergeable: 'UNKNOWN' })]); + expect(pr!.pullRequest.mergeable).toBeUndefined(); + }); + + it('preserves isDraft', async () => { + const [pr] = await callAdapter([makeNode({ isDraft: true })]); + expect(pr!.pullRequest.isDraft).toBe(true); + }); + + it('handles a null author (deleted account)', async () => { + const [pr] = await callAdapter([makeNode({ author: null })]); + expect(pr!.pullRequest.authorLogin).toBe(''); + expect(pr!.pullRequest.authorAvatarUrl).toBe(''); + }); + + it('latestReviews APPROVED + COMMENTED → reviewStatus approved', async () => { + const [pr] = await callAdapter([ + makeNode({ + latestReviews: { + nodes: [ + { state: 'APPROVED', author: { login: 'a' } }, + { state: 'COMMENTED', author: { login: 'b' } }, + ], + }, + }), + ]); + expect(pr!.pullRequest.reviewStatus).toBe('approved'); + }); + + it('latestReviews CHANGES_REQUESTED wins over APPROVED', async () => { + const [pr] = await callAdapter([ + makeNode({ + latestReviews: { + nodes: [ + { state: 'APPROVED', author: { login: 'a' } }, + { state: 'CHANGES_REQUESTED', author: { login: 'b' } }, + ], + }, + }), + ]); + expect(pr!.pullRequest.reviewStatus).toBe('changesRequested'); + }); + + it('reviewRequests with User and Team → flat string array', async () => { + const [pr] = await callAdapter([ + makeNode({ + reviewRequests: { + nodes: [ + { requestedReviewer: { __typename: 'User', login: 'alice' } }, + { requestedReviewer: { __typename: 'Team', name: 'platform' } }, + { requestedReviewer: { __typename: 'Mannequin' } }, + { requestedReviewer: null }, + ], + }, + }), + ]); + expect(pr!.pullRequest.requestedReviewers).toEqual(['alice', 'platform']); + }); + + it('all-green check runs → overallStatus green', async () => { + const [pr] = await callAdapter([ + makeNode( + makeRollup([ + { + __typename: 'CheckRun', + name: 'build', + status: 'COMPLETED', + conclusion: 'SUCCESS', + checkSuite: { databaseId: 1 }, + }, + ]), + ), + ]); + expect(pr!.overallStatus).toBe('green'); + expect(pr!.passedCount).toBe(1); + expect(pr!.totalCheckCount).toBe(1); + }); + + it('mixed contexts → failed names + suite ids, pending, passed, skipped', async () => { + const [pr] = await callAdapter([ + makeNode( + makeRollup( + [ + { + __typename: 'CheckRun', + name: 'build', + status: 'COMPLETED', + conclusion: 'FAILURE', + checkSuite: { databaseId: 11 }, + }, + { + __typename: 'CheckRun', + name: 'pending-test', + status: 'IN_PROGRESS', + conclusion: null, + checkSuite: { databaseId: 22 }, + }, + { + __typename: 'CheckRun', + name: 'lint', + status: 'COMPLETED', + conclusion: 'SUCCESS', + checkSuite: { databaseId: 33 }, + }, + { + __typename: 'CheckRun', + name: 'optional', + status: 'COMPLETED', + conclusion: 'SKIPPED', + checkSuite: { databaseId: 44 }, + }, + ], + 'FAILURE', + ), + ), + ]); + expect(pr!.overallStatus).toBe('red'); + expect(pr!.failedCheckNames).toEqual(['build']); + expect(pr!.failedCheckSuiteIds).toEqual([11]); + expect(pr!.pendingCheckNames).toEqual(['pending-test']); + expect(pr!.passedCount).toBe(1); + expect(pr!.skippedCount).toBe(1); + expect(pr!.totalCheckCount).toBe(4); + }); + + it('timed_out conclusion counts as failed with its suite id', async () => { + const [pr] = await callAdapter([ + makeNode( + makeRollup( + [ + { + __typename: 'CheckRun', + name: 'slow', + status: 'COMPLETED', + conclusion: 'TIMED_OUT', + checkSuite: { databaseId: 7 }, + }, + ], + 'FAILURE', + ), + ), + ]); + expect(pr!.failedCheckNames).toEqual(['slow']); + expect(pr!.failedCheckSuiteIds).toEqual([7]); + expect(pr!.overallStatus).toBe('red'); + }); + + it('failed check with null checkSuite contributes name but no suite id', async () => { + const [pr] = await callAdapter([ + makeNode( + makeRollup( + [ + { + __typename: 'CheckRun', + name: 'orphan', + status: 'COMPLETED', + conclusion: 'FAILURE', + checkSuite: null, + }, + ], + 'FAILURE', + ), + ), + ]); + expect(pr!.failedCheckNames).toEqual(['orphan']); + expect(pr!.failedCheckSuiteIds).toEqual([]); + }); + + it('cancelled check among successes stays green (cancelled is non-blocking)', async () => { + const [pr] = await callAdapter([ + makeNode( + makeRollup( + [ + { + __typename: 'CheckRun', + name: 'build', + status: 'COMPLETED', + conclusion: 'SUCCESS', + checkSuite: { databaseId: 1 }, + }, + { + __typename: 'CheckRun', + name: 'cancelled-shard', + status: 'COMPLETED', + conclusion: 'CANCELLED', + checkSuite: { databaseId: 1 }, + }, + ], + // GitHub's rollup would say FAILURE here — we deliberately derive + // from contexts instead, preserving the REST-era behavior. + 'FAILURE', + ), + ), + ]); + expect(pr!.overallStatus).toBe('green'); + expect(pr!.failedCheckNames).toEqual([]); + }); + + it('WAITING status counts as pending', async () => { + const [pr] = await callAdapter([ + makeNode( + makeRollup( + [ + { + __typename: 'CheckRun', + name: 'gated-deploy', + status: 'WAITING', + conclusion: null, + checkSuite: { databaseId: 5 }, + }, + ], + 'PENDING', + ), + ), + ]); + expect(pr!.pendingCheckNames).toEqual(['gated-deploy']); + expect(pr!.overallStatus).toBe('yellow'); + }); + + it('legacy StatusContext FAILURE contributes name but no suite id', async () => { + const [pr] = await callAdapter([ + makeNode( + makeRollup( + [{ __typename: 'StatusContext', context: 'continuous-integration', state: 'FAILURE' }], + 'FAILURE', + ), + ), + ]); + expect(pr!.failedCheckNames).toEqual(['continuous-integration']); + expect(pr!.failedCheckSuiteIds).toEqual([]); + expect(pr!.overallStatus).toBe('red'); + }); + + it('legacy StatusContext SUCCESS and PENDING map to passed and pending', async () => { + const [pr] = await callAdapter([ + makeNode( + makeRollup( + [ + { __typename: 'StatusContext', context: 'ci/done', state: 'SUCCESS' }, + { __typename: 'StatusContext', context: 'ci/waiting', state: 'PENDING' }, + ], + 'PENDING', + ), + ), + ]); + expect(pr!.passedCount).toBe(1); + expect(pr!.pendingCheckNames).toEqual(['ci/waiting']); + expect(pr!.overallStatus).toBe('yellow'); + expect(pr!.totalCheckCount).toBe(2); + }); + + it('maps merged/closed timestamps through', async () => { + const [pr] = await callAdapter([ + makeNode({ + state: 'MERGED', + mergedAt: '2026-05-02T00:00:00Z', + closedAt: '2026-05-02T00:00:00Z', + }), + ]); + expect(pr!.pullRequest.state).toBe('merged'); + expect(pr!.pullRequest.mergedAt).toBe('2026-05-02T00:00:00Z'); + expect(pr!.pullRequest.closedAt).toBe('2026-05-02T00:00:00Z'); + }); +}); diff --git a/src/BorgDock.Tauri/src/services/github/__tests__/rate-limit.test.ts b/src/BorgDock.Tauri/src/services/github/__tests__/rate-limit.test.ts index 4cd384736..e8dd396ee 100644 --- a/src/BorgDock.Tauri/src/services/github/__tests__/rate-limit.test.ts +++ b/src/BorgDock.Tauri/src/services/github/__tests__/rate-limit.test.ts @@ -1,6 +1,6 @@ -import { describe, it, expect, vi, beforeEach } from 'vitest'; import { renderHook, waitFor } from '@testing-library/react'; -import { useGitHubRateLimit, fetchGitHubRateLimit } from '../rate-limit'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { fetchGitHubRateLimit, useGitHubRateLimit } from '../rate-limit'; vi.mock('../auth', () => ({ getGitHubToken: vi.fn().mockResolvedValue('fake-token'), @@ -14,17 +14,27 @@ vi.mock('@/stores/settings-store', () => ({ })); describe('fetchGitHubRateLimit', () => { - beforeEach(() => { vi.restoreAllMocks(); }); + beforeEach(() => { + vi.restoreAllMocks(); + }); - it('parses GitHub rate_limit response', async () => { + it('parses both REST and GraphQL pools from the rate_limit response', async () => { const fetchMock = vi.spyOn(global, 'fetch').mockResolvedValue( new Response( - JSON.stringify({ resources: { core: { used: 123, limit: 5000, reset: 1717000000 } } }), + JSON.stringify({ + resources: { + core: { used: 123, limit: 5000, reset: 1717000000 }, + graphql: { used: 42, limit: 5000, reset: 1717000600 }, + }, + }), { status: 200 }, ), ); const rl = await fetchGitHubRateLimit('token'); - expect(rl).toEqual({ used: 123, limit: 5000, resetAt: 1717000000 }); + expect(rl).toEqual({ + rest: { used: 123, limit: 5000, resetAt: 1717000000 }, + graphql: { used: 42, limit: 5000, resetAt: 1717000600 }, + }); expect(fetchMock).toHaveBeenCalledWith( 'https://api.github.com/rate_limit', expect.objectContaining({ @@ -33,28 +43,47 @@ describe('fetchGitHubRateLimit', () => { ); }); - it('throws on non-OK response', async () => { + it('defaults a missing graphql resource to zero usage', async () => { vi.spyOn(global, 'fetch').mockResolvedValue( - new Response('forbidden', { status: 403 }), + new Response( + JSON.stringify({ resources: { core: { used: 9, limit: 5000, reset: 1717000000 } } }), + { status: 200 }, + ), ); + const rl = await fetchGitHubRateLimit('token'); + expect(rl.graphql).toEqual({ used: 0, limit: 5000, resetAt: 0 }); + }); + + it('throws on non-OK response', async () => { + vi.spyOn(global, 'fetch').mockResolvedValue(new Response('forbidden', { status: 403 })); await expect(fetchGitHubRateLimit('token')).rejects.toThrow(/403/); }); }); describe('useGitHubRateLimit', () => { - beforeEach(() => { vi.restoreAllMocks(); }); + beforeEach(() => { + vi.restoreAllMocks(); + }); it('returns null initially, then the parsed rate limit', async () => { vi.spyOn(global, 'fetch').mockResolvedValue( new Response( - JSON.stringify({ resources: { core: { used: 10, limit: 5000, reset: 1717000000 } } }), + JSON.stringify({ + resources: { + core: { used: 10, limit: 5000, reset: 1717000000 }, + graphql: { used: 3, limit: 5000, reset: 1717000300 }, + }, + }), { status: 200 }, ), ); const { result } = renderHook(() => useGitHubRateLimit(60000)); expect(result.current).toBeNull(); await waitFor(() => { - expect(result.current).toEqual({ used: 10, limit: 5000, resetAt: 1717000000 }); + expect(result.current).toEqual({ + rest: { used: 10, limit: 5000, resetAt: 1717000000 }, + graphql: { used: 3, limit: 5000, resetAt: 1717000300 }, + }); }); }); }); diff --git a/src/BorgDock.Tauri/src/services/github/aggregate.ts b/src/BorgDock.Tauri/src/services/github/aggregate.ts index f7121c620..2d6a0013c 100644 --- a/src/BorgDock.Tauri/src/services/github/aggregate.ts +++ b/src/BorgDock.Tauri/src/services/github/aggregate.ts @@ -6,9 +6,13 @@ export function aggregatePrWithChecks( ): PullRequestWithChecks { const overallStatus = computeOverallStatus(checkRuns); - const failedCheckNames = checkRuns - .filter((c) => c.conclusion === 'failure' || c.conclusion === 'timed_out') - .map((c) => c.name); + const failedRuns = checkRuns.filter( + (c) => c.conclusion === 'failure' || c.conclusion === 'timed_out', + ); + const failedCheckNames = failedRuns.map((c) => c.name); + const failedCheckSuiteIds = failedRuns + .map((c) => c.checkSuiteId) + .filter((id): id is number => typeof id === 'number' && id > 0); const pendingCheckNames = checkRuns .filter((c) => c.status === 'in_progress' || c.status === 'queued') @@ -22,12 +26,13 @@ export function aggregatePrWithChecks( return { pullRequest: pr, - checks: checkRuns, overallStatus, failedCheckNames, + failedCheckSuiteIds, pendingCheckNames, passedCount, skippedCount, + totalCheckCount: checkRuns.length, }; } diff --git a/src/BorgDock.Tauri/src/services/github/client.ts b/src/BorgDock.Tauri/src/services/github/client.ts index 572177471..cb9d799ef 100644 --- a/src/BorgDock.Tauri/src/services/github/client.ts +++ b/src/BorgDock.Tauri/src/services/github/client.ts @@ -20,7 +20,10 @@ const log = createLogger('github'); export class GitHubClient { private readonly getToken: () => Promise; private readonly etagCache = new Map(); - private rateLimit: RateLimit = { remaining: -1, total: -1, reset: null }; + // REST and GraphQL draw from separate 5000/h pools — track them separately + // so neither poisons the other's reading (the polling loop is GraphQL-only). + private restRateLimit: RateLimit = { remaining: -1, total: -1, reset: null }; + private graphqlRateLimit: RateLimit = { remaining: -1, total: -1, reset: null }; private _freshCount = 0; private _pollStartCount = 0; @@ -38,12 +41,21 @@ export class GitHubClient { return this._freshCount > this._pollStartCount; } + /** REST (core) pool. */ getRateLimit(): RateLimit { - return { ...this.rateLimit }; + return { ...this.restRateLimit }; } + /** GraphQL pool — populated by graphql() from headers and the inline rateLimit field. */ + getGraphqlRateLimit(): RateLimit { + return { ...this.graphqlRateLimit }; + } + + /** True when either pool is running low — adaptive polling doubles its interval. */ get isRateLimitLow(): boolean { - return this.rateLimit.remaining >= 0 && this.rateLimit.remaining < 500; + const rest = this.restRateLimit.remaining; + const graphql = this.graphqlRateLimit.remaining; + return (rest >= 0 && rest < 500) || (graphql >= 0 && graphql < 500); } /** Populate the in-memory ETag cache from persisted entries (call on startup). */ @@ -78,7 +90,7 @@ export class GitHubClient { log.warn('GET 304 with no cached entry', { path, durationMs }); this.etagCache.delete(url); const retryResponse = await this.fetchWithRetry(url); - this.parseRateLimitHeaders(retryResponse); + this.parseRestRateLimitHeaders(retryResponse); if (!retryResponse.ok) { throw new GitHubApiError( `GitHub API error: ${retryResponse.status} ${retryResponse.statusText}`, @@ -125,7 +137,7 @@ export class GitHubClient { path, status: response.status, durationMs, - rateLimitRemaining: this.rateLimit.remaining, + rateLimitRemaining: this.restRateLimit.remaining, cached: !!etag, }); @@ -165,7 +177,7 @@ export class GitHubClient { body: JSON.stringify(body), }); - this.parseRateLimitHeaders(response); + this.parseRestRateLimitHeaders(response); this.checkResponseForAuthErrors(response, 'POST', path); if (!response.ok) { @@ -193,7 +205,7 @@ export class GitHubClient { body: JSON.stringify(body), }); - this.parseRateLimitHeaders(response); + this.parseRestRateLimitHeaders(response); this.checkResponseForAuthErrors(response, 'PUT', path); if (!response.ok) { @@ -225,7 +237,7 @@ export class GitHubClient { body: JSON.stringify(body), }); - this.parseRateLimitHeaders(response); + this.parseRestRateLimitHeaders(response); this.checkResponseForAuthErrors(response, 'PATCH', path); if (!response.ok) { @@ -239,6 +251,9 @@ export class GitHubClient { } async graphql(query: string, variables?: Record): Promise { + const start = performance.now(); + const queryName = query.match(/(?:query|mutation)\s+(\w+)/)?.[1] ?? 'anonymous'; + log.info('graphql start', { query: queryName }); const token = await this.getToken(); const response = await this.fetchWithTimeout('https://api.github.com/graphql', { @@ -251,9 +266,21 @@ export class GitHubClient { body: JSON.stringify({ query, variables }), }); - this.parseRateLimitHeaders(response); + this.parseGraphqlRateLimitFromHeaders(response); + const durationMs = Math.round(performance.now() - start); if (!response.ok) { + log.error('graphql failed', { query: queryName, status: response.status, durationMs }); + if (response.status === 403 && this.graphqlRateLimit.remaining === 0) { + throw new GitHubRateLimitError( + `GitHub API rate limit exceeded. Resets at ${this.graphqlRateLimit.reset?.toISOString() ?? 'unknown'}.`, + this.graphqlRateLimit.reset, + ); + } + if (response.status === 401 || response.status === 403) { + invalidateGitHubTokenCache(); + throw new GitHubAuthError(`GitHub API authentication failed (${response.status}).`); + } throw new GitHubApiError( `GitHub GraphQL error: ${response.status} ${response.statusText}`, response.status, @@ -262,17 +289,27 @@ export class GitHubClient { const result = await response.json(); if (result.errors?.length > 0) { + log.error('graphql errors', { query: queryName, errors: result.errors, durationMs }); throw new GitHubApiError(`GraphQL error: ${result.errors[0].message}`, 422); } + // Body rateLimit (when the query requests it) is the authoritative point + // count — parse it after headers so it wins. + const cost = this.parseGraphqlRateLimitFromBody(result.data); + log.info('graphql ok', { + query: queryName, + durationMs, + cost, + rateLimitRemaining: this.graphqlRateLimit.remaining, + }); return result.data as T; } private checkResponseForAuthErrors(response: Response, _method: string, _path: string): void { - if (response.status === 403 && this.rateLimit.remaining === 0) { + if (response.status === 403 && this.restRateLimit.remaining === 0) { throw new GitHubRateLimitError( - `GitHub API rate limit exceeded. Resets at ${this.rateLimit.reset?.toISOString() ?? 'unknown'}.`, - this.rateLimit.reset, + `GitHub API rate limit exceeded. Resets at ${this.restRateLimit.reset?.toISOString() ?? 'unknown'}.`, + this.restRateLimit.reset, ); } if (response.status === 401 || response.status === 403) { @@ -305,11 +342,11 @@ export class GitHubClient { } const response = await this.fetchWithTimeout(url, { headers }); - this.parseRateLimitHeaders(response); + this.parseRestRateLimitHeaders(response); // Handle rate limit exhaustion - if (response.status === 403 && this.rateLimit.remaining === 0) { - const resetTime = this.rateLimit.reset; + if (response.status === 403 && this.restRateLimit.remaining === 0) { + const resetTime = this.restRateLimit.reset; if (resetTime && attempt < maxRetries) { const waitMs = Math.max(0, resetTime.getTime() - Date.now()); const cappedWait = Math.min(waitMs, 120_000); @@ -385,24 +422,67 @@ export class GitHubClient { } } - private parseRateLimitHeaders(response: Response): void { + private parseRestRateLimitHeaders(response: Response): void { const remaining = response.headers.get('X-RateLimit-Remaining'); if (remaining) { const val = parseInt(remaining, 10); - if (!Number.isNaN(val)) this.rateLimit.remaining = val; + if (!Number.isNaN(val)) this.restRateLimit.remaining = val; } const limit = response.headers.get('X-RateLimit-Limit'); if (limit) { const val = parseInt(limit, 10); - if (!Number.isNaN(val)) this.rateLimit.total = val; + if (!Number.isNaN(val)) this.restRateLimit.total = val; } const reset = response.headers.get('X-RateLimit-Reset'); if (reset) { const val = parseInt(reset, 10); - if (!Number.isNaN(val)) this.rateLimit.reset = new Date(val * 1000); + if (!Number.isNaN(val)) this.restRateLimit.reset = new Date(val * 1000); + } + } + + /** GraphQL responses carry the same X-RateLimit-* headers, but for the GraphQL pool. */ + private parseGraphqlRateLimitFromHeaders(response: Response): void { + const remaining = response.headers.get('X-RateLimit-Remaining'); + if (remaining) { + const val = parseInt(remaining, 10); + if (!Number.isNaN(val)) this.graphqlRateLimit.remaining = val; + } + + const limit = response.headers.get('X-RateLimit-Limit'); + if (limit) { + const val = parseInt(limit, 10); + if (!Number.isNaN(val)) this.graphqlRateLimit.total = val; + } + + const reset = response.headers.get('X-RateLimit-Reset'); + if (reset) { + const val = parseInt(reset, 10); + if (!Number.isNaN(val)) this.graphqlRateLimit.reset = new Date(val * 1000); + } + } + + /** + * Parse the inline `rateLimit { remaining limit resetAt cost }` field that + * queries may request at the root. Returns the reported query cost (points) + * for logging, if present. + */ + private parseGraphqlRateLimitFromBody(data: unknown): number | undefined { + if (!data || typeof data !== 'object') return undefined; + const rl = ( + data as { + rateLimit?: { remaining?: number; limit?: number; resetAt?: string; cost?: number }; + } + ).rateLimit; + if (!rl) return undefined; + if (typeof rl.remaining === 'number') this.graphqlRateLimit.remaining = rl.remaining; + if (typeof rl.limit === 'number') this.graphqlRateLimit.total = rl.limit; + if (typeof rl.resetAt === 'string') { + const d = new Date(rl.resetAt); + if (!Number.isNaN(d.getTime())) this.graphqlRateLimit.reset = d; } + return typeof rl.cost === 'number' ? rl.cost : undefined; } } diff --git a/src/BorgDock.Tauri/src/services/github/index.ts b/src/BorgDock.Tauri/src/services/github/index.ts index c0e43435e..3834d87a9 100644 --- a/src/BorgDock.Tauri/src/services/github/index.ts +++ b/src/BorgDock.Tauri/src/services/github/index.ts @@ -9,6 +9,7 @@ export { export type { RateLimit } from './client'; export { GitHubApiError, GitHubAuthError, GitHubClient, GitHubRateLimitError } from './client'; export { mergePullRequest, postComment, submitReview, toggleDraft } from './mutations'; +export { POLL_OPEN_PRS_QUERY, pollOpenPrsAggregate } from './polling'; export { aggregateReviewStatus, getClosedPRs, diff --git a/src/BorgDock.Tauri/src/services/github/polling.ts b/src/BorgDock.Tauri/src/services/github/polling.ts new file mode 100644 index 000000000..96d9dbbd8 --- /dev/null +++ b/src/BorgDock.Tauri/src/services/github/polling.ts @@ -0,0 +1,328 @@ +import { createLogger } from '@/services/logger'; +import type { CheckRun, PullRequest, PullRequestWithChecks } from '@/types'; +import { aggregatePrWithChecks } from './aggregate'; +import type { GitHubClient } from './client'; +import { aggregateReviewStatus } from './pulls'; + +const log = createLogger('github:polling'); + +/** + * Single-query replacement for the REST polling fan-out: open PRs, their + * check rollups, latest reviews per user, and counts — one request per repo + * per poll cycle instead of ~22. + * + * `rateLimit` sits at the query root (it is a Query field, not a Repository + * field) so `GitHubClient.graphql` can read the GraphQL pool from the body. + */ +export const POLL_OPEN_PRS_QUERY = /* GraphQL */ ` + query PollOpenPrs($owner: String!, $repo: String!) { + repository(owner: $owner, name: $repo) { + pullRequests(states: OPEN, first: 100, orderBy: { field: UPDATED_AT, direction: DESC }) { + totalCount + nodes { + number + title + body + url + state + isDraft + mergeable + createdAt + updatedAt + closedAt + mergedAt + additions + deletions + changedFiles + author { + login + avatarUrl + } + baseRefName + headRefName + headRefOid + labels(first: 20) { + nodes { + name + } + } + reviewRequests(first: 20) { + nodes { + requestedReviewer { + __typename + ... on User { + login + } + ... on Team { + name + } + } + } + } + commits(last: 1) { + totalCount + nodes { + commit { + statusCheckRollup { + state + contexts(first: 100) { + pageInfo { + hasNextPage + } + nodes { + __typename + ... on CheckRun { + name + status + conclusion + checkSuite { + databaseId + } + } + ... on StatusContext { + context + state + } + } + } + } + } + } + } + latestReviews(first: 50) { + nodes { + state + author { + login + } + } + } + comments { + totalCount + } + reviewThreads { + totalCount + } + } + } + } + rateLimit { + remaining + limit + resetAt + cost + } + } +`; + +type GqlMergeable = 'MERGEABLE' | 'CONFLICTING' | 'UNKNOWN'; + +interface GqlActor { + login: string; + avatarUrl: string; +} + +/** User exposes `login`, Team exposes `name`; other reviewer types come back bare. */ +interface GqlRequestedReviewer { + __typename: string; + login?: string; + name?: string; +} + +interface GqlReview { + state: string; + author: { login: string } | null; +} + +interface GqlCheckRunContext { + __typename: 'CheckRun'; + name: string; + status: string; + conclusion: string | null; + checkSuite: { databaseId: number | null } | null; +} + +interface GqlStatusContext { + __typename: 'StatusContext'; + context: string; + state: string; +} + +type GqlContext = GqlCheckRunContext | GqlStatusContext; + +interface GqlStatusCheckRollup { + state: string; + contexts: { + pageInfo?: { hasNextPage: boolean }; + nodes: GqlContext[]; + }; +} + +interface GqlPrNode { + number: number; + title: string; + body: string | null; + url: string; + state: string; + isDraft: boolean; + mergeable: GqlMergeable; + createdAt: string; + updatedAt: string; + closedAt: string | null; + mergedAt: string | null; + additions: number; + deletions: number; + changedFiles: number; + author: GqlActor | null; + baseRefName: string; + headRefName: string; + headRefOid: string; + labels: { nodes: Array<{ name: string }> } | null; + reviewRequests: { nodes: Array<{ requestedReviewer: GqlRequestedReviewer | null }> } | null; + commits: { + totalCount: number; + nodes: Array<{ commit: { statusCheckRollup: GqlStatusCheckRollup | null } }>; + }; + latestReviews: { nodes: GqlReview[] } | null; + comments: { totalCount: number }; + reviewThreads: { totalCount: number }; +} + +interface PollResponse { + repository: { + pullRequests: { + totalCount: number; + nodes: GqlPrNode[]; + }; + } | null; +} + +/** + * Fetch all open PRs for a repo with their check summaries in one GraphQL + * call, returning the same `PullRequestWithChecks` shape the REST fan-out + * produced. The summary fields are derived by mapping rollup contexts to + * `CheckRun`-shaped objects and reusing {@link aggregatePrWithChecks}, so + * status semantics (including cancelled-as-non-blocking) stay identical. + */ +export async function pollOpenPrsAggregate( + client: GitHubClient, + owner: string, + repo: string, +): Promise { + const data = await client.graphql(POLL_OPEN_PRS_QUERY, { owner, repo }); + // `repository` is null when the repo is inaccessible; the optional chain also + // tolerates malformed 200 bodies (e.g. stubbed network in tests) so a poll + // cycle degrades to "no data" instead of TypeError-ing. + const pullRequests = data?.repository?.pullRequests; + if (!pullRequests) { + log.warn('poll response missing repository.pullRequests', { owner, repo }); + return []; + } + if (pullRequests.totalCount > 100) { + log.warn('PR list truncated to 100', { + owner, + repo, + totalCount: pullRequests.totalCount, + }); + } + return (pullRequests.nodes ?? []).map((node) => mapNode(node, owner, repo)); +} + +function mapNode(node: GqlPrNode, owner: string, repo: string): PullRequestWithChecks { + const rollup = node.commits.nodes[0]?.commit.statusCheckRollup ?? null; + if (rollup?.contexts.pageInfo?.hasNextPage) { + log.warn('check contexts truncated to 100 — counts may be low', { + owner, + repo, + pr: node.number, + }); + } + const checkRuns = (rollup?.contexts.nodes ?? []).map(contextToCheckRunLite); + + const requestedReviewers = (node.reviewRequests?.nodes ?? []) + .map((rr) => rr.requestedReviewer?.login ?? rr.requestedReviewer?.name ?? '') + .filter((s) => s.length > 0); + + const reviewStatus = aggregateReviewStatus( + (node.latestReviews?.nodes ?? []).map((r) => ({ + state: r.state, + user: r.author ? { login: r.author.login, avatar_url: '' } : null, + })), + ); + + const pr: PullRequest = { + number: node.number, + title: node.title, + headRef: node.headRefName, + headSha: node.headRefOid, + baseRef: node.baseRefName, + authorLogin: node.author?.login ?? '', + authorAvatarUrl: node.author?.avatarUrl ?? '', + state: node.state.toLowerCase(), + createdAt: node.createdAt, + updatedAt: node.updatedAt, + isDraft: node.isDraft, + mergeable: mapMergeable(node.mergeable), + htmlUrl: node.url, + body: node.body ?? '', + repoOwner: owner, + repoName: repo, + reviewStatus, + commentCount: node.comments.totalCount + node.reviewThreads.totalCount, + labels: (node.labels?.nodes ?? []).map((l) => l.name).filter((n) => n.length > 0), + additions: node.additions, + deletions: node.deletions, + changedFiles: node.changedFiles, + commitCount: node.commits.totalCount, + mergedAt: node.mergedAt ?? undefined, + closedAt: node.closedAt ?? undefined, + requestedReviewers, + }; + + return aggregatePrWithChecks(pr, checkRuns); +} + +function mapMergeable(state: GqlMergeable): boolean | undefined { + if (state === 'MERGEABLE') return true; + if (state === 'CONFLICTING') return false; + return undefined; // UNKNOWN — GitHub is still computing mergeability +} + +/** GraphQL check statuses with no REST equivalent — all "not started yet". */ +const PENDING_CHECK_STATUSES = new Set(['waiting', 'pending', 'requested']); + +/** + * Map a rollup context to the REST `CheckRun` shape so the existing + * aggregation rules apply unchanged. These lites never reach the UI — the + * aggregate keeps only derived counts/names/suite-ids. + */ +function contextToCheckRunLite(ctx: GqlContext): CheckRun { + if (ctx.__typename === 'CheckRun') { + const status = ctx.status.toLowerCase(); + return { + id: 0, + name: ctx.name, + status: PENDING_CHECK_STATUSES.has(status) ? 'queued' : status, + conclusion: ctx.conclusion?.toLowerCase(), + htmlUrl: '', + checkSuiteId: ctx.checkSuite?.databaseId ?? 0, + }; + } + + const state = ctx.state.toUpperCase(); + const isPending = state === 'PENDING' || state === 'EXPECTED'; + return { + id: 0, + name: ctx.context, + status: isPending ? 'queued' : 'completed', + conclusion: isPending + ? undefined + : state === 'SUCCESS' + ? 'success' + : state === 'FAILURE' || state === 'ERROR' + ? 'failure' + : undefined, + htmlUrl: '', + checkSuiteId: 0, + }; +} diff --git a/src/BorgDock.Tauri/src/services/github/pulls.ts b/src/BorgDock.Tauri/src/services/github/pulls.ts index 39aa62a24..de02e8a9c 100644 --- a/src/BorgDock.Tauri/src/services/github/pulls.ts +++ b/src/BorgDock.Tauri/src/services/github/pulls.ts @@ -137,16 +137,18 @@ export async function getOpenPRs( /** * Targeted single-PR refetch — fetches detail + reviews + checks for one PR * and returns the same {@link PullRequestWithChecks} shape the polling loop - * produces. Used by mutation handlers (merge / bypass / close / toggle draft / - * submit review) to reflect server-truth immediately instead of waiting for - * the next poll cycle. + * produces, plus the raw check runs (the PR shape only carries derived + * counts; the detail window's Checks tab needs the full runs). Used by + * mutation handlers (merge / bypass / close / toggle draft / submit review) + * to reflect server-truth immediately instead of waiting for the next poll + * cycle. */ export async function getPRWithChecks( client: GitHubClient, owner: string, repo: string, number: number, -): Promise { +): Promise<{ pr: PullRequestWithChecks; checks: CheckRun[] }> { const [detail, reviews] = await Promise.all([ client.get(`repos/${owner}/${repo}/pulls/${number}`), client.get(`repos/${owner}/${repo}/pulls/${number}/reviews`), @@ -167,7 +169,7 @@ export async function getPRWithChecks( }); } - return aggregatePrWithChecks(pr, checks); + return { pr: aggregatePrWithChecks(pr, checks), checks }; } export async function getClosedPRs( diff --git a/src/BorgDock.Tauri/src/services/github/rate-limit.ts b/src/BorgDock.Tauri/src/services/github/rate-limit.ts index cdacd7bda..cfddf5502 100644 --- a/src/BorgDock.Tauri/src/services/github/rate-limit.ts +++ b/src/BorgDock.Tauri/src/services/github/rate-limit.ts @@ -1,6 +1,6 @@ import { useEffect, useState } from 'react'; -import { getGitHubToken } from './auth'; import { useSettingsStore } from '@/stores/settings-store'; +import { getGitHubToken } from './auth'; export interface RateLimit { used: number; @@ -8,11 +8,18 @@ export interface RateLimit { resetAt: number; // epoch seconds (matches GitHub's response) } +/** REST (core) and GraphQL draw from separate 5000/h pools on GitHub's side. */ +export interface DualRateLimit { + rest: RateLimit; + graphql: RateLimit; +} + /** - * Hits GET /rate_limit on the GitHub REST API and returns the `core` resource - * usage. Token must be supplied (caller resolves via getGitHubToken). + * Hits GET /rate_limit on the GitHub REST API and returns both the `core` + * (REST) and `graphql` resource usage. Token must be supplied (caller + * resolves via getGitHubToken). */ -export async function fetchGitHubRateLimit(token: string): Promise { +export async function fetchGitHubRateLimit(token: string): Promise { const resp = await fetch('https://api.github.com/rate_limit', { headers: { Authorization: `Bearer ${token}`, @@ -25,10 +32,18 @@ export async function fetchGitHubRateLimit(token: string): Promise { } const body = await resp.json(); const core = body?.resources?.core ?? {}; + const graphql = body?.resources?.graphql ?? {}; return { - used: Number(core.used ?? 0), - limit: Number(core.limit ?? 5000), - resetAt: Number(core.reset ?? 0), + rest: { + used: Number(core.used ?? 0), + limit: Number(core.limit ?? 5000), + resetAt: Number(core.reset ?? 0), + }, + graphql: { + used: Number(graphql.used ?? 0), + limit: Number(graphql.limit ?? 5000), + resetAt: Number(graphql.reset ?? 0), + }, }; } @@ -37,9 +52,9 @@ export async function fetchGitHubRateLimit(token: string): Promise { * Returns null until the first fetch lands. Resolves the token via * `getGitHubToken(pat)`, where pat comes from settings if set. */ -export function useGitHubRateLimit(pollMs = 60_000): RateLimit | null { +export function useGitHubRateLimit(pollMs = 60_000): DualRateLimit | null { const pat = useSettingsStore((s) => s.settings.gitHub.personalAccessToken); - const [rl, setRl] = useState(null); + const [rl, setRl] = useState(null); useEffect(() => { let cancelled = false; @@ -55,7 +70,10 @@ export function useGitHubRateLimit(pollMs = 60_000): RateLimit | null { } fetchOnce(); const t = setInterval(fetchOnce, pollMs); - return () => { cancelled = true; clearInterval(t); }; + return () => { + cancelled = true; + clearInterval(t); + }; }, [pat, pollMs]); return rl; diff --git a/src/BorgDock.Tauri/src/services/merge-score.ts b/src/BorgDock.Tauri/src/services/merge-score.ts index 3a73cb589..1b5b6b2dd 100644 --- a/src/BorgDock.Tauri/src/services/merge-score.ts +++ b/src/BorgDock.Tauri/src/services/merge-score.ts @@ -2,7 +2,7 @@ import type { PullRequestWithChecks } from '@/types'; export function computeMergeScore(pr: PullRequestWithChecks): number { let score = 0; - const relevant = pr.checks.length - pr.skippedCount; + const relevant = pr.totalCheckCount - pr.skippedCount; // CI checks (25%) if (relevant > 0) { diff --git a/src/BorgDock.Tauri/src/stores/__tests__/pr-store.test.ts b/src/BorgDock.Tauri/src/stores/__tests__/pr-store.test.ts index 8d392637a..fb0806c48 100644 --- a/src/BorgDock.Tauri/src/stores/__tests__/pr-store.test.ts +++ b/src/BorgDock.Tauri/src/stores/__tests__/pr-store.test.ts @@ -44,12 +44,13 @@ function makePr(overrides: { commitCount: 1, requestedReviewers: [], }, - checks: [], overallStatus: overrides.overallStatus ?? 'green', failedCheckNames: overrides.overallStatus === 'red' ? ['ci'] : [], + failedCheckSuiteIds: overrides.overallStatus === 'red' ? [1] : [], pendingCheckNames: overrides.overallStatus === 'yellow' ? ['ci'] : [], passedCount: overrides.overallStatus === 'green' ? 1 : 0, skippedCount: 0, + totalCheckCount: (overrides.overallStatus ?? 'green') === 'gray' ? 0 : 1, }; } diff --git a/src/BorgDock.Tauri/src/stores/__tests__/quick-review-store.test.ts b/src/BorgDock.Tauri/src/stores/__tests__/quick-review-store.test.ts index 748af7712..289ee06ab 100644 --- a/src/BorgDock.Tauri/src/stores/__tests__/quick-review-store.test.ts +++ b/src/BorgDock.Tauri/src/stores/__tests__/quick-review-store.test.ts @@ -29,12 +29,13 @@ function makePr(number: number): PullRequestWithChecks { commitCount: 1, requestedReviewers: [], }, - checks: [], overallStatus: 'green', failedCheckNames: [], + failedCheckSuiteIds: [], pendingCheckNames: [], passedCount: 1, skippedCount: 0, + totalCheckCount: 1, }; } diff --git a/src/BorgDock.Tauri/src/stores/pr-store.ts b/src/BorgDock.Tauri/src/stores/pr-store.ts index 00faefb47..79c4afbdc 100644 --- a/src/BorgDock.Tauri/src/stores/pr-store.ts +++ b/src/BorgDock.Tauri/src/stores/pr-store.ts @@ -8,7 +8,7 @@ import { sortByPriority, } from '@/services/priority-scoring'; import { computeTeamReviewLoad, type ReviewerLoad } from '@/services/team-review-load'; -import type { PullRequestWithChecks } from '@/types'; +import type { CheckRun, PullRequestWithChecks } from '@/types'; const log = createLogger('pr-store'); @@ -21,6 +21,9 @@ export interface PrRefreshedDetail { repo: string; number: number; pr: PullRequestWithChecks | null; // null = no longer accessible / removed + /** Raw check runs from the refresh fetch. Absent on optimistic updates + * (e.g. mark-merged) — listeners keep their current runs in that case. */ + checks?: CheckRun[]; } export type PrFilter = @@ -94,11 +97,7 @@ interface PrState extends DerivedCache { * matching entry in {@link pullRequests}; closed/merged PRs are moved to * {@link closedPullRequests}. Also broadcasts a `borgdock-pr-refreshed` * DOM event for any window-local listeners (e.g. pop-out detail panels). */ - refreshPr: ( - owner: string, - repo: string, - number: number, - ) => Promise; + refreshPr: (owner: string, repo: string, number: number) => Promise; /** Synchronously mark a PR as merged in the local store and dispatch * PR_REFRESHED_EVENT, without round-tripping through GitHub. Used by * pr-actions to give the PR detail window an instant visual update @@ -487,8 +486,11 @@ export const usePrStore = create()((set, get) => ({ if (!client) return null; let fresh: PullRequestWithChecks; + let freshChecks: CheckRun[]; try { - fresh = await getPRWithChecks(client, owner, repo, number); + const result = await getPRWithChecks(client, owner, repo, number); + fresh = result.pr; + freshChecks = result.checks; } catch (err) { log.warn('refreshPr fetch failed', { error: String(err), owner, repo, number }); return null; @@ -541,7 +543,7 @@ export const usePrStore = create()((set, get) => ({ }); if (typeof document !== 'undefined') { - const detail: PrRefreshedDetail = { owner, repo, number, pr: fresh }; + const detail: PrRefreshedDetail = { owner, repo, number, pr: fresh, checks: freshChecks }; document.dispatchEvent(new CustomEvent(PR_REFRESHED_EVENT, { detail })); } return fresh; diff --git a/src/BorgDock.Tauri/src/test-support/__tests__/test-seed.test.ts b/src/BorgDock.Tauri/src/test-support/__tests__/test-seed.test.ts index 1fb11f185..d6b834769 100644 --- a/src/BorgDock.Tauri/src/test-support/__tests__/test-seed.test.ts +++ b/src/BorgDock.Tauri/src/test-support/__tests__/test-seed.test.ts @@ -35,12 +35,13 @@ const DESIGN_PRS: PullRequestWithChecks[] = [ commitCount: 1, requestedReviewers: [], }, - checks: [], overallStatus: 'green', failedCheckNames: [], + failedCheckSuiteIds: [], pendingCheckNames: [], passedCount: 0, skippedCount: 0, + totalCheckCount: 0, }, { pullRequest: { @@ -67,12 +68,13 @@ const DESIGN_PRS: PullRequestWithChecks[] = [ commitCount: 1, requestedReviewers: [], }, - checks: [], overallStatus: 'green', failedCheckNames: [], + failedCheckSuiteIds: [], pendingCheckNames: [], passedCount: 0, skippedCount: 0, + totalCheckCount: 0, }, ]; diff --git a/src/BorgDock.Tauri/src/test-utils/make-pr.ts b/src/BorgDock.Tauri/src/test-utils/make-pr.ts index 14cac5101..da39f2a7e 100644 --- a/src/BorgDock.Tauri/src/test-utils/make-pr.ts +++ b/src/BorgDock.Tauri/src/test-utils/make-pr.ts @@ -40,11 +40,12 @@ export function makePr(number: number, overrides: MakePrOverrides = {}): PullReq commitCount: 1, requestedReviewers, }, - checks: [], overallStatus, failedCheckNames: overallStatus === 'red' ? ['build'] : [], + failedCheckSuiteIds: overallStatus === 'red' ? [1] : [], pendingCheckNames: [], passedCount: overallStatus === 'green' ? 1 : 0, skippedCount: 0, + totalCheckCount: overallStatus === 'green' || overallStatus === 'red' ? 1 : 0, }; } diff --git a/src/BorgDock.Tauri/src/types/pull-request.ts b/src/BorgDock.Tauri/src/types/pull-request.ts index 284b2ec79..d2d8179f1 100644 --- a/src/BorgDock.Tauri/src/types/pull-request.ts +++ b/src/BorgDock.Tauri/src/types/pull-request.ts @@ -1,5 +1,3 @@ -import type { CheckRun } from './check-run'; - export type ReviewStatus = 'none' | 'pending' | 'commented' | 'approved' | 'changesRequested'; export type OverallStatus = 'red' | 'yellow' | 'green' | 'gray'; @@ -35,12 +33,14 @@ export interface PullRequest { export interface PullRequestWithChecks { pullRequest: PullRequest; - checks: CheckRun[]; overallStatus: OverallStatus; failedCheckNames: string[]; + /** Check-suite ids parallel to failedCheckNames (suite-less entries dropped) — used for rerun actions. */ + failedCheckSuiteIds: number[]; pendingCheckNames: string[]; passedCount: number; skippedCount: number; + totalCheckCount: number; } export interface PullRequestCommit { diff --git a/src/BorgDock.Tauri/tests/e2e/helpers/seed.ts b/src/BorgDock.Tauri/tests/e2e/helpers/seed.ts index 4cf91dd7a..ed0acb282 100644 --- a/src/BorgDock.Tauri/tests/e2e/helpers/seed.ts +++ b/src/BorgDock.Tauri/tests/e2e/helpers/seed.ts @@ -145,12 +145,13 @@ function makePr(overrides: { }; return { pullRequest: base, - checks: [], overallStatus: overrides.overallStatus ?? 'green', failedCheckNames: overrides.overallStatus === 'red' ? ['ci'] : [], + failedCheckSuiteIds: overrides.overallStatus === 'red' ? [1] : [], pendingCheckNames: [], passedCount: 1, skippedCount: 0, + totalCheckCount: 1, }; } diff --git a/src/BorgDock.Tauri/tests/e2e/helpers/test-utils.ts b/src/BorgDock.Tauri/tests/e2e/helpers/test-utils.ts index 0a8d0b857..a4fd49e48 100644 --- a/src/BorgDock.Tauri/tests/e2e/helpers/test-utils.ts +++ b/src/BorgDock.Tauri/tests/e2e/helpers/test-utils.ts @@ -98,7 +98,22 @@ export async function bootApp( body: JSON.stringify({ login: 'test-user' }), }); } - // Default: empty list (most GitHub endpoints expect arrays). + // The polling loop POSTs one PollOpenPrs GraphQL query per repo per + // cycle — answer with a well-formed empty repository so cycles no-op + // (PR data is seeded via mock IPC / test-seed, not the network). + if (url.endsWith('/graphql')) { + return route.fulfill({ + status: 200, + contentType: 'application/json', + body: JSON.stringify({ + data: { + repository: { pullRequests: { totalCount: 0, nodes: [] } }, + rateLimit: { remaining: 5000, limit: 5000, resetAt: '2026-01-01T00:00:00Z', cost: 1 }, + }, + }), + }); + } + // Default: empty list (most GitHub REST endpoints expect arrays). return route.fulfill({ status: 200, contentType: 'application/json',