From 9a15d648063ebdca7f437c2a58222ec8fee8f004 Mon Sep 17 00:00:00 2001 From: Will Gordon Date: Tue, 18 Aug 2026 16:31:21 -0400 Subject: [PATCH 1/2] fix(deps): reorders title-parsing patterns, closes label-fallback gaps Fixes findings from /pr-review on PR #135: - Reorders genericMatch (from/to semver-diff) before crateMatch/dockerMatch so a title combining a crate/docker-tag keyword with from...to phrasing classifies via the correct semver-diff path instead of having the lazy capture group absorb the from-clause into the package name. - Narrows the generic single-target fallback's capture group from .+? to a single non-whitespace token so a human-authored, multi-word title (reachable via the label-only dependency admission path) isn't misclassified as a dependency bump. - Adds digest/pin/maintenance to needsBodyFallback's label check so it mirrors depCategory's label list, avoiding a redundant GraphQL body-fetch for a PR already classified via label. - Adds regression tests for all of the above, plus boundary tests for scoped npm packages and slash-containing Go module paths. --- src/app/lib/dependency-detection.ts | 28 ++++-- .../dashboard/DependenciesTab.test.tsx | 52 +++++++++++ tests/lib/dependency-detection.test.ts | 91 ++++++++++++++++++- 3 files changed, 157 insertions(+), 14 deletions(-) diff --git a/src/app/lib/dependency-detection.ts b/src/app/lib/dependency-detection.ts index 2fd5f6a3..8182ca7e 100644 --- a/src/app/lib/dependency-detection.ts +++ b/src/app/lib/dependency-detection.ts @@ -145,6 +145,17 @@ export function extractVersionInfo(title: string): VersionInfo | null { return { packageName: actionMatch[1]!, to: actionMatch[2]! }; } + // Generic "from A to B" anywhere. Checked before crateMatch/dockerMatch below so that a title + // combining the literal "crate"/"docker tag" keyword with "from...to" phrasing (e.g. "Update + // crate pyo3 from 0.29.0 to 0.29.1") is classified via the correct semver-diff path instead of + // having crateMatch/dockerMatch's lazy capture group absorb the "from" clause into the package + // name (their (.+?) group expands until the first " to " match, which would land after the + // "from" version, not after the package name). + const genericMatch = /\bfrom\s+([\w.\-+]+)\s+to\s+([\w.\-+]+)/i.exec(body); + if (genericMatch) { + return { from: genericMatch[1]!, to: genericMatch[2]!, updateType: semverUpdateType(genericMatch[1]!, genericMatch[2]!) ?? undefined }; + } + // "Update (rust) crate X to vY" const crateMatch = /^Update\s+(?:rust\s+)?crate\s+(.+?)\s+to\s+(v?[\w.\-+]+)/i.exec(body); if (crateMatch && /^v?\d/.test(crateMatch[2]!)) { @@ -157,14 +168,13 @@ export function extractVersionInfo(title: string): VersionInfo | null { return { packageName: dockerMatch[1]!, to: dockerMatch[2]! }; } - // Generic "from A to B" anywhere - const genericMatch = /\bfrom\s+([\w.\-+]+)\s+to\s+([\w.\-+]+)/i.exec(body); - if (genericMatch) { - return { from: genericMatch[1]!, to: genericMatch[2]!, updateType: semverUpdateType(genericMatch[1]!, genericMatch[2]!) ?? undefined }; - } - - // Generic "Update X to vY" (last resort, single-target version only) - const singleTargetMatch = /^Update\s+(.+?)\s+to\s+(v?[\w.\-+]+)$/i.exec(body); + // Generic "Update X to vY" (last resort, single-target version only). The package-name group + // requires a single whitespace-free token (rather than any characters) so a human-authored, + // multi-word title like "Update the docs to v2" — reachable via isDependencyPr()'s label-only + // admission path, which needs no bot-like title at all — isn't misclassified as a dependency + // bump. Real bot-generated titles for this fallback (e.g. "update renovate to v44", "update + // node to v24.18.1") are single-token package names and are unaffected. + const singleTargetMatch = /^Update\s+(\S+)\s+to\s+(v?[\w.\-+]+)$/i.exec(body); if (singleTargetMatch && /^v?\d/.test(singleTargetMatch[2]!)) { return { packageName: singleTargetMatch[1]!, to: singleTargetMatch[2]! }; } @@ -249,7 +259,7 @@ export function needsBodyFallback(pr: PullRequest): boolean { if (/lock\s*file\s+maintenance/i.test(pr.title)) return false; for (const l of pr.labels) { const name = l.name.toLowerCase(); - if (name === "major" || name === "minor" || name === "patch") return false; + if (name === "major" || name === "minor" || name === "patch" || name === "digest" || name === "pin" || name === "maintenance") return false; } return true; } diff --git a/tests/components/dashboard/DependenciesTab.test.tsx b/tests/components/dashboard/DependenciesTab.test.tsx index b9b3df9a..9fef0397 100644 --- a/tests/components/dashboard/DependenciesTab.test.tsx +++ b/tests/components/dashboard/DependenciesTab.test.tsx @@ -445,6 +445,18 @@ describe("DependenciesTab — updateType filter", () => { expect(screen.getByText("Lock file maintenance")).toBeDefined(); }); + it("filters to digest only when updateType=digest is set", () => { + const digest = makeMergeablePR({ + title: "chore(deps): refresh vendored dependencies", + labels: [{ name: "digest", color: "1a7f37" }], + }); + const patch = makeMergeablePR({ title: "Bump axios from 0.27.1 to 0.27.2" }); + setTabFilter("dependencies", "updateType", "digest"); + renderTab({ pullRequests: [digest, patch] }); + expect(screen.getByText("Refresh vendored dependencies")).toBeDefined(); + expect(screen.queryByText("axios: 0.27.1 → 0.27.2")).toBeNull(); + }); + it("uses label as fallback when title has no version info", () => { const pr = makeMergeablePR({ title: "chore(deps): update dependency foo to v2", @@ -511,6 +523,46 @@ describe("DependenciesTab — category classification", () => { expect(categories).toEqual(["pin", "digest", "patch"]); }); + it("sorts all 7 categories in strict risk order: maintenance, pin, digest, patch, minor, major, other", () => { + const prMaintenance = makeMergeablePR({ id: 7101, title: "chore(deps): lock file maintenance" }); + const prPin = makeMergeablePR({ id: 7102, title: "chore(deps): pin dependencies" }); + const prDigest = makeMergeablePR({ + id: 7103, + title: "chore(deps): refresh vendored dependencies", + labels: [{ name: "digest", color: "1a7f37" }], + }); + const prPatch = makeMergeablePR({ id: 7104, title: "Bump axios from 0.27.1 to 0.27.2" }); + const prMinor = makeMergeablePR({ id: 7105, title: "Bump lodash from 4.16.0 to 4.17.0" }); + const prMajor = makeMergeablePR({ id: 7106, title: "Bump react from 17.0.0 to 18.0.0" }); + // Regression guard: an unparseable title with no matching labels must classify as "other" + // (unknown risk) and sort last, not fall back to "maintenance" (verified-safe) and sort first. + const prOther = makeMergeablePR({ + id: 7107, + title: "chore(deps): refresh vendored dependencies for widgets", + }); + + // Deliberately shuffled input order to prove the sort — not insertion order — determines output. + renderTab({ pullRequests: [prOther, prMajor, prMinor, prPatch, prDigest, prPin, prMaintenance] }); + + const items = screen.getAllByRole("listitem"); + const order = items.map((item) => { + const text = item.textContent ?? ""; + if (text.includes("Lock file maintenance")) return "maintenance"; + if (text.includes("Pin dependencies")) return "pin"; + if (text.includes("Refresh vendored dependencies for widgets")) return "other"; + if (text.includes("Refresh vendored dependencies")) return "digest"; + if (text.includes("axios")) return "patch"; + if (text.includes("lodash")) return "minor"; + if (text.includes("react")) return "major"; + return "unknown"; + }); + + expect(order).toEqual(["maintenance", "pin", "digest", "patch", "minor", "major", "other"]); + // "other" renders no risk badge (Show when={category !== "other"}) — confirm it's not mislabeled "maintenance". + expect(screen.queryByText("maintenance")).not.toBeNull(); + expect(screen.getAllByText("maintenance")).toHaveLength(1); + }); + it("PR with unparseable title and no matching labels renders as 'other' (hidden badge), not 'maintenance'", () => { const pr = makeMergeablePR({ title: "chore(deps): refresh vendored dependencies" }); renderTab({ pullRequests: [pr] }); diff --git a/tests/lib/dependency-detection.test.ts b/tests/lib/dependency-detection.test.ts index a0abf98b..cb7a3a79 100644 --- a/tests/lib/dependency-detection.test.ts +++ b/tests/lib/dependency-detection.test.ts @@ -260,15 +260,45 @@ describe("extractVersionInfo", () => { expect(result).toBeNull(); }); + it("extracts a scoped npm package name via the generic single-target fallback", () => { + const result = extractVersionInfo("chore(deps): update @actions/checkout to v5"); + expect(result).toEqual({ packageName: "@actions/checkout", to: "v5" }); + }); + + it("extracts a slash-containing Go module path via the generic single-target fallback", () => { + const result = extractVersionInfo("chore(deps): update golang.org/x/net to v0.20.0"); + expect(result).toEqual({ packageName: "golang.org/x/net", to: "v0.20.0" }); + }); + + it("does not match the generic single-target fallback for a human-authored multi-word title with no dependency-specific signal", () => { + // "Update the docs to v2" satisfies the version-shape check ("v2" starts with a digit) and + // would have matched the old (.+?) capture group, producing a spurious + // { packageName: "the docs", to: "v2" }. It's reachable via isDependencyPr()'s label-only + // admission path (a PR manually labeled "dependencies" needs no bot-like title). The + // single-token (\S+) requirement rejects it since the captured text would have to include a + // space. + const result = extractVersionInfo("Update the docs to v2"); + expect(result).toBeNull(); + }); + it("still prefers genericMatch's semver-diff path over the new single-target patterns when 'from' is present", () => { - // Synthetic "Update X from A to B"-style title (no real crate-manager sample of this shape was - // found) without the literal "crate"/"docker tag" keywords, since a title that combines those - // keywords with "from...to" hits the documented absorption caveat on the crate/docker patterns - // themselves (see Task 2 Step 1's ASSUMPTION note) rather than exercising this fallback-ordering - // regression check. + // Title has no "crate"/"docker tag" keyword, so crateMatch/dockerMatch never get a chance to + // run here — this just confirms genericMatch's bare "from...to" phrasing still wins over the + // generic single-target fallback. const result = extractVersionInfo("chore(deps): update pyo3 from 0.29.0 to 0.29.1"); expect(result).toEqual({ from: "0.29.0", to: "0.29.1", updateType: "patch" }); }); + + it("prefers genericMatch's semver-diff path over crateMatch even when the title combines the 'crate' keyword with 'from...to' phrasing", () => { + // Regression test for the crate/docker absorption bug: genericMatch is checked before + // crateMatch/dockerMatch precisely so that a title combining the literal "crate" keyword with + // "from...to" phrasing is classified via the correct semver-diff path. Before this ordering + // fix, crateMatch's lazy capture group would have absorbed the "from" clause into the package + // name (packageName: "pyo3 from 0.29.0"), since its lazy (.+?) group expands until the first + // " to " match, which occurs after "0.29.0", not after "pyo3". + const result = extractVersionInfo("chore(deps): update crate pyo3 from 0.29.0 to 0.29.1"); + expect(result).toEqual({ from: "0.29.0", to: "0.29.1", updateType: "patch" }); + }); }); describe("stripVersionSpecifier", () => { @@ -799,4 +829,55 @@ describe("needsBodyFallback", () => { }); expect(needsBodyFallback(pr)).toBe(false); }); + + it("returns true for a crate-matched title with no updateType and no risk label", () => { + const pr = makePullRequest({ + title: "chore(deps): update rust crate pyo3 to v0.29.1", + nodeId: "PR_abc", + }); + expect(needsBodyFallback(pr)).toBe(true); + }); + + it("returns true for a Docker-tag-matched title with no updateType and no risk label", () => { + const pr = makePullRequest({ + title: "chore(deps): update node Docker tag to v24.18.1", + nodeId: "PR_abc", + }); + expect(needsBodyFallback(pr)).toBe(true); + }); + + it("returns true for a generic single-target-matched title with no updateType and no risk label", () => { + const pr = makePullRequest({ + title: "chore(deps): update renovate to v44", + nodeId: "PR_abc", + }); + expect(needsBodyFallback(pr)).toBe(true); + }); + + it("returns false when digest label present (mirrors depCategory's label list)", () => { + const pr = makePullRequest({ + title: "chore(deps): update node Docker tag to v24.18.1", + nodeId: "PR_abc", + labels: [{ name: "digest", color: "ff0000" }], + }); + expect(needsBodyFallback(pr)).toBe(false); + }); + + it("returns false when pin label present (mirrors depCategory's label list)", () => { + const pr = makePullRequest({ + title: "chore(deps): update rust crate pyo3 to v0.29.1", + nodeId: "PR_abc", + labels: [{ name: "pin", color: "00ff00" }], + }); + expect(needsBodyFallback(pr)).toBe(false); + }); + + it("returns false when maintenance label present (mirrors depCategory's label list)", () => { + const pr = makePullRequest({ + title: "chore(deps): update renovate to v44", + nodeId: "PR_abc", + labels: [{ name: "maintenance", color: "0000ff" }], + }); + expect(needsBodyFallback(pr)).toBe(false); + }); }); From 0bcd57152193a0caaec5ca61d7409e1a42241754 Mon Sep 17 00:00:00 2001 From: Will Gordon Date: Tue, 18 Aug 2026 16:33:13 -0400 Subject: [PATCH 2/2] refactor(deps): extracts shared body-fetch helper, adds failure cooldown MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes remaining findings from /pr-review + this session's own /quality-gate on PR #135: - Extracts fetchNodeBodiesBatched() as a shared helper for fetchDashboardIssueBodies/fetchDepPRBodies, removing ~20 lines of duplicated batching/timeout/error-reporting boilerplate. Both now return { bodies, failedIds } instead of a bare Map. - Adds a 5-minute failure-cooldown Map in DashboardPage.tsx so a persistently-failing dependency PR isn't re-fetched (burning a full 20s timeout + an unbounded Sentry event) every poll cycle — mirrors the existing _repoLastTargeted/TARGETED_COOLDOWN_MS pattern in poll.ts. The cooldown-prune step runs on every effect evaluation (not only when something needs fetching), so a PR that fails, leaves the dependency set, and reopens isn't stuck on a stale entry. - Restores always-on console.debug logging in both files (reverting an earlier DEV-gating pass) to match this codebase's dominant console.debug/info convention (most existing calls — auth.ts, notifications.ts, OAuth callbacks — already ship unconditionally to production) and to preserve the original implementation plan's production-diagnostic strategy for a still-open reactivity-bug investigation. - Rewords a debug log message that leaked internal authoring commentary, and rewrites a test comment that referenced an external planning-document section number. - Adds a banner comment matching api.ts's established section-header convention. - Adds tests: full 7-category sort-order regression, a Digest filter functional test, an integration test proving the hung-request guard releases and a later poll retries, and three cooldown-specific integration tests (same-PR exclusion within the cooldown window, cooldown-entry pruning when a PR leaves the dependency set, and pruning when no other PR triggers a fetch in between). --- .../components/dashboard/DashboardPage.tsx | 46 ++- src/app/services/api.ts | 182 ++++++------ tests/components/DashboardPage.test.tsx | 281 ++++++++++++++++++ tests/services/api-dashboard-bodies.test.ts | 46 +-- tests/services/api-dep-pr-bodies.test.ts | 45 +-- 5 files changed, 468 insertions(+), 132 deletions(-) diff --git a/src/app/components/dashboard/DashboardPage.tsx b/src/app/components/dashboard/DashboardPage.tsx index db63861a..b070c1f8 100644 --- a/src/app/components/dashboard/DashboardPage.tsx +++ b/src/app/components/dashboard/DashboardPage.tsx @@ -179,6 +179,13 @@ const [depMeta, setDepMeta] = createSignal>(loa let _fetchingDashboardBodies = false; let _fetchingDepBodies = false; +// Skip retrying a dep-PR body fetch that errored/timed out on its last attempt until +// this cooldown elapses — prevents burning a full GRAPHQL_BODY_FETCH_TIMEOUT_MS timeout, +// an undeduped Sentry event, and a repeated user notification every poll cycle for as +// long as the underlying failure (e.g. a secondary-rate-limit stall) persists. +export const DEP_BODY_FAILURE_COOLDOWN_MS = 5 * 60 * 1000; +const _depBodyFailureCooldown = new Map(); + // Clear dashboard data and stop polling on logout to prevent cross-user data leakage onAuthCleared(() => { resetDashboardData(); @@ -193,6 +200,7 @@ onAuthCleared(() => { localStorage.removeItem?.(DEP_META_STORAGE_KEY); _fetchingDashboardBodies = false; _fetchingDepBodies = false; + _depBodyFailureCooldown.clear(); resetAbandonedPatternCache(); const coord = _coordinator(); if (coord) { @@ -1194,7 +1202,7 @@ export default function DashboardPage() { if (relevant.length === 0) return; const nodeIds = relevant.map((di) => di.nodeId); - const bodyMap = await fetchDashboardIssueBodies(octokit, nodeIds); + const { bodies: bodyMap } = await fetchDashboardIssueBodies(octokit, nodeIds); const newAbandonedMap = new Map(); const newUrlMap = new Map(); @@ -1223,7 +1231,7 @@ export default function DashboardPage() { createEffect(() => { if (!config.dependencies.enabled) return; if (_fetchingDepBodies) { - console.debug("[dashboard] depBodies effect: skipped — fetch already in flight (this run's tracked deps are now narrowed to config.dependencies.enabled only)"); + console.debug("[dashboard] depBodies effect: skipped — fetch already in flight"); return; } const octokit = getClient(); @@ -1232,7 +1240,23 @@ export default function DashboardPage() { const meta = depMeta(); const depPrs = dependencyPullRequests(); const visibleDepPrs = visibleDependencyPullRequests(); - const toFetch = visibleDepPrs.filter((pr) => !meta.has(pr.id) && needsBodyFallback(pr)); + const now = Date.now(); + + // Prune cooldown entries for PRs no longer in the dependency set on every + // effect run, regardless of whether anything needs fetching this cycle — + // otherwise a PR that fails, leaves the set, and reopens before some OTHER + // PR happens to trigger a fetch would incorrectly stay excluded on a stale + // cooldown entry for up to DEP_BODY_FAILURE_COOLDOWN_MS. + const depPrIds = new Set(depPrs.map((pr) => pr.id)); + for (const k of [..._depBodyFailureCooldown.keys()]) { + if (!depPrIds.has(k)) _depBodyFailureCooldown.delete(k); + } + + const toFetch = visibleDepPrs.filter((pr) => { + if (meta.has(pr.id) || !needsBodyFallback(pr)) return false; + const failedAt = _depBodyFailureCooldown.get(pr.id); + return !failedAt || now - failedAt >= DEP_BODY_FAILURE_COOLDOWN_MS; + }); if (toFetch.length === 0) { console.debug("[dashboard] depBodies effect: nothing to fetch", { metaSize: meta.size, visibleDepPrCount: visibleDepPrs.length }); return; @@ -1244,8 +1268,19 @@ export default function DashboardPage() { void (async () => { try { const nodeIds = toFetch.map((pr) => pr.nodeId!); - const bodyMap = await fetchDepPRBodies(octokit, nodeIds); - console.debug(`[dashboard] depBodies effect: fetch resolved after ${Date.now() - effectStart}ms`, { requested: toFetch.length, returned: bodyMap.size }); + const { bodies: bodyMap, failedIds } = await fetchDepPRBodies(octokit, nodeIds); + console.debug(`[dashboard] depBodies effect: fetch resolved after ${Date.now() - effectStart}ms`, { requested: toFetch.length, returned: bodyMap.size, failed: failedIds.size }); + + // Record/clear per-PR cooldown so a batch that errored or timed out isn't + // retried again until DEP_BODY_FAILURE_COOLDOWN_MS has elapsed. + for (const pr of toFetch) { + if (pr.nodeId && failedIds.has(pr.nodeId)) { + _depBodyFailureCooldown.set(pr.id, now); + } else { + _depBodyFailureCooldown.delete(pr.id); + } + } + if (bodyMap.size === 0) return; const merged = new Map(meta); @@ -1254,7 +1289,6 @@ export default function DashboardPage() { if (parsed) merged.set(id, parsed); } // Prune entries for PRs no longer in the dependency set - const depPrIds = new Set(depPrs.map((pr) => pr.id)); for (const k of [...merged.keys()]) { if (!depPrIds.has(k)) merged.delete(k); } diff --git a/src/app/services/api.ts b/src/app/services/api.ts index bf5329b0..43d086b4 100644 --- a/src/app/services/api.ts +++ b/src/app/services/api.ts @@ -1162,9 +1162,9 @@ export async function fetchPREnrichment( return { enrichments, errors }; } -// Shared timeout guard for body-fetch GraphQL calls. Prevents a hung request -// (e.g. octokit's secondary-rate-limit retry logic stalling indefinitely) -// from wedging the caller's fetch-in-progress gate. +// ── Body-fetch timeout guard ──────────────────────────────────────────────── +// Prevents a hung request (e.g. octokit's secondary-rate-limit retry logic +// stalling indefinitely) from wedging the caller's fetch-in-progress gate. export const GRAPHQL_BODY_FETCH_TIMEOUT_MS = 20_000; @@ -1185,70 +1185,113 @@ export function raceWithTimeout(promise: Promise, ms: number, controller: }); } -// ── Dashboard issue body fetch ──────────────────────────────────────────────── - -const DASHBOARD_ISSUE_BODIES_QUERY = ` - query($ids: [ID!]!) { - nodes(ids: $ids) { - ... on Issue { id body } - } - rateLimit { cost limit remaining resetAt } - } -`; - -interface DashboardIssueBodiesResponse { - nodes: Array<{ id: string; body: string | null } | null>; +interface NodeBodiesResponse { + nodes: Array; rateLimit?: GraphQLRateLimit; } -/** Fetches issue bodies for Dashboard issues by node ID (single nodes() batch query). */ -export async function fetchDashboardIssueBodies( +export interface NodeBodiesFetchResult { + bodies: Map; + /** Requested node IDs belonging to a batch that errored or timed out. */ + failedIds: Set; +} + +/** + * Shared batched-fetch helper for GraphQL `nodes()` queries that pull a single + * field (e.g. issue/PR body text) for a set of node IDs. Chunks ids into + * NODES_BATCH_SIZE batches, races each batch via raceWithTimeout, and reports + * failures via console.warn + Sentry + a single deduped pushNotification. + * + * `mapNode` lets each caller decide how to fold a raw node into the result map + * (e.g. whether to keep or skip a null body) without duplicating the batching, + * timeout, rate-limit, and error-reporting plumbing. Batches that error or time + * out contribute their requested ids to `failedIds` so callers can avoid + * retrying a persistently-failing id on every poll cycle. + */ +async function fetchNodeBodiesBatched( octokit: GitHubOctokit, - issueNodeIds: string[] -): Promise> { - const result = new Map(); - if (issueNodeIds.length === 0) return result; + nodeIds: string[], + query: string, + source: string, + notificationMessage: string, + mapNode: (node: TNode, bodies: Map) => void, +): Promise> { + const bodies = new Map(); + const failedIds = new Set(); + if (nodeIds.length === 0) return { bodies, failedIds }; - const batches = chunkArray(issueNodeIds, NODES_BATCH_SIZE); - let hadFailure = false; + const batches = chunkArray(nodeIds, NODES_BATCH_SIZE); await Promise.allSettled(batches.map(async (batch) => { const batchStart = Date.now(); - console.debug(`[api] dashboardBodies batch started (${batch.length} ids) at ${batchStart}`); + console.debug(`[api] ${source} batch started (${batch.length} ids) at ${batchStart}`); const controller = new AbortController(); try { const response = await raceWithTimeout( - octokit.graphql( - DASHBOARD_ISSUE_BODIES_QUERY, - { ids: batch, request: { apiSource: "dashboardBodies", signal: controller.signal } } + octokit.graphql>( + query, + { ids: batch, request: { apiSource: source, signal: controller.signal } } ), GRAPHQL_BODY_FETCH_TIMEOUT_MS, controller, ); if (response.rateLimit) updateGraphqlRateLimit(response.rateLimit); for (const node of response.nodes) { - if (!node || !node.id) continue; - result.set(node.id, node.body); + if (!node) continue; + mapNode(node, bodies); } } catch (err) { - hadFailure = true; - console.warn("[api] dashboardBodies batch failed or timed out:", err); - Sentry.captureException(err, { tags: { source: "dashboardBodies" } }); + for (const id of batch) failedIds.add(id); + console.warn(`[api] ${source} batch failed or timed out:`, err); + Sentry.captureException(err, { tags: { source } }); const partialErr = err && typeof err === "object" && "data" in err && err.data && typeof err.data === "object" - ? (err.data as Partial) + ? (err.data as Partial>) : null; if (partialErr?.rateLimit) updateGraphqlRateLimit(partialErr.rateLimit); - // Partial failures return null bodies — callers handle missing entries gracefully } finally { - console.debug(`[api] dashboardBodies batch settled after ${Date.now() - batchStart}ms`); + console.debug(`[api] ${source} batch settled after ${Date.now() - batchStart}ms`); } })); - if (hadFailure && getClient() === octokit) { - pushNotification("dashboardBodies", "Some dependency dashboard data could not be loaded", "warning"); + if (failedIds.size > 0 && getClient() === octokit) { + pushNotification(source, notificationMessage, "warning"); } - return result; + return { bodies, failedIds }; +} + +// ── Dashboard issue body fetch ──────────────────────────────────────────────── + +const DASHBOARD_ISSUE_BODIES_QUERY = ` + query($ids: [ID!]!) { + nodes(ids: $ids) { + ... on Issue { id body } + } + rateLimit { cost limit remaining resetAt } + } +`; + +interface DashboardIssueBodyNode { + id: string; + body: string | null; +} + +/** Fetches issue bodies for Dashboard issues by node ID (single nodes() batch query). */ +export async function fetchDashboardIssueBodies( + octokit: GitHubOctokit, + issueNodeIds: string[] +): Promise> { + return fetchNodeBodiesBatched( + octokit, + issueNodeIds, + DASHBOARD_ISSUE_BODIES_QUERY, + "dashboardBodies", + "Some dependency dashboard data could not be loaded", + (node, bodies) => { + if (!node.id) return; + bodies.set(node.id, node.body); + }, + ); } // ── Dependency PR body fetch ───────────────────────────────────────────────── @@ -1262,57 +1305,26 @@ const DEP_PR_BODIES_QUERY = ` } `; -interface DepPRBodiesResponse { - nodes: Array<{ databaseId: number; body: string | null } | null>; - rateLimit?: GraphQLRateLimit; +interface DepPRBodyNode { + databaseId: number; + body: string | null; } export async function fetchDepPRBodies( octokit: GitHubOctokit, prNodeIds: string[] -): Promise> { - const result = new Map(); - if (prNodeIds.length === 0) return result; - - const batches = chunkArray(prNodeIds, NODES_BATCH_SIZE); - let hadFailure = false; - await Promise.allSettled(batches.map(async (batch) => { - const batchStart = Date.now(); - console.debug(`[api] depPRBodies batch started (${batch.length} ids) at ${batchStart}`); - const controller = new AbortController(); - try { - const response = await raceWithTimeout( - octokit.graphql( - DEP_PR_BODIES_QUERY, - { ids: batch, request: { apiSource: "depPRBodies", signal: controller.signal } } - ), - GRAPHQL_BODY_FETCH_TIMEOUT_MS, - controller, - ); - if (response.rateLimit) updateGraphqlRateLimit(response.rateLimit); - for (const node of response.nodes) { - if (!node || node.databaseId == null || !node.body) continue; - result.set(node.databaseId, node.body); - } - } catch (err) { - hadFailure = true; - console.warn("[api] depPRBodies batch failed or timed out:", err); - Sentry.captureException(err, { tags: { source: "depPRBodies" } }); - const partialErr = - err && typeof err === "object" && "data" in err && err.data && typeof err.data === "object" - ? (err.data as Partial) - : null; - if (partialErr?.rateLimit) updateGraphqlRateLimit(partialErr.rateLimit); - } finally { - console.debug(`[api] depPRBodies batch settled after ${Date.now() - batchStart}ms`); - } - })); - - if (hadFailure && getClient() === octokit) { - pushNotification("depPRBodies", "Some dependency PR types could not be determined — badges may be missing", "warning"); - } - - return result; +): Promise> { + return fetchNodeBodiesBatched( + octokit, + prNodeIds, + DEP_PR_BODIES_QUERY, + "depPRBodies", + "Some dependency PR types could not be determined — badges may be missing", + (node, bodies) => { + if (node.databaseId == null || !node.body) return; + bodies.set(node.databaseId, node.body); + }, + ); } /** diff --git a/tests/components/DashboardPage.test.tsx b/tests/components/DashboardPage.test.tsx index cdd5c445..0c931b44 100644 --- a/tests/components/DashboardPage.test.tsx +++ b/tests/components/DashboardPage.test.tsx @@ -5,6 +5,7 @@ import userEvent from "@testing-library/user-event"; import { makeIssue, makePullRequest, makeWorkflowRun } from "../helpers/index"; import type { DashboardData } from "../../src/app/services/poll"; import type { HotPRStatusUpdate, HotWorkflowRunUpdate } from "../../src/app/services/api"; +import { GRAPHQL_BODY_FETCH_TIMEOUT_MS } from "../../src/app/services/api"; const mockLocationReplace = vi.fn(); @@ -97,6 +98,7 @@ let capturedOnTargetedData: ((data: DashboardData, affectedRepos: string[]) => v // so the module-level _coordinator variable is always fresh (null) per test. let DashboardPage: typeof import("../../src/app/components/dashboard/DashboardPage").default; let _resetHasFetchedFresh: typeof import("../../src/app/components/dashboard/DashboardPage")._resetHasFetchedFresh; +let DEP_BODY_FAILURE_COOLDOWN_MS: typeof import("../../src/app/components/dashboard/DashboardPage").DEP_BODY_FAILURE_COOLDOWN_MS; let pollService: typeof import("../../src/app/services/poll"); let authStore: typeof import("../../src/app/stores/auth"); let viewStore: typeof import("../../src/app/stores/view"); @@ -157,6 +159,7 @@ beforeEach(async () => { const dashboardModule = await import("../../src/app/components/dashboard/DashboardPage"); DashboardPage = dashboardModule.default; _resetHasFetchedFresh = dashboardModule._resetHasFetchedFresh; + DEP_BODY_FAILURE_COOLDOWN_MS = dashboardModule.DEP_BODY_FAILURE_COOLDOWN_MS; pollService = await import("../../src/app/services/poll"); authStore = await import("../../src/app/stores/auth"); viewStore = await import("../../src/app/stores/view"); @@ -2719,6 +2722,284 @@ describe("DashboardPage — pruneJiraCustomOrder on refresh", () => { }); }); +// ── Dependencies tab — depBodies fetch guard recovers from a hung request ─────── + +describe("DashboardPage — depBodies fetch guard recovers from a hung GraphQL request", () => { + it("releases the _fetchingDepBodies guard after the GraphQL timeout fires, so a later poll cycle retries the fetch", async () => { + vi.useFakeTimers(); + try { + const githubService = await import("../../src/app/services/github"); + // Never resolves — reproduces a hung GraphQL request: without + // raceWithTimeout, awaiting this promise would never settle, and + // the effect's finally block (which releases _fetchingDepBodies) would + // never run, permanently wedging the fetch-in-progress guard. + const graphqlSpy = vi.fn(() => new Promise(() => {})); + vi.mocked(githubService.getClient).mockReturnValue( + { graphql: graphqlSpy } as unknown as ReturnType + ); + + const depPR1 = makePullRequest({ + id: 100, + nodeId: "PR_A", + repoFullName: "owner/repo", + title: "Update dependency some-pkg", + userLogin: "dependabot[bot]", + headRef: "dependabot/npm_and_yarn/some-pkg", + }); + vi.mocked(pollService.fetchAllData).mockResolvedValue({ + issues: [], + pullRequests: [depPR1], + workflowRuns: [], + errors: [], + }); + + render(() => ); + + // vi.waitFor polls using real (un-mocked) timers even while fake timers + // are installed, so this settles as soon as the pending microtasks from + // fetchAllData's resolved mock and Solid's reactive depBodies effect + // propagate — no timer advance is needed to reach the first graphql call. + await vi.waitFor(() => { + expect(graphqlSpy).toHaveBeenCalledTimes(1); + }, { timeout: 5000 }); + + // Advance past the body-fetch timeout. raceWithTimeout's internal + // setTimeout fires, aborts the controller, and rejects the race; + // fetchDepPRBodies swallows that per-batch failure (Promise.allSettled) + // and resolves with an empty bodies Map — so the depBodies effect's + // `finally` block runs and releases the _fetchingDepBodies guard. + await vi.advanceTimersByTimeAsync(GRAPHQL_BODY_FETCH_TIMEOUT_MS); + + // A second dependency PR arrives on a later poll cycle. If the guard + // was truly released, the depBodies effect fires again and retries the + // fetch (a second graphql call). Without the timeout fix, a hung request + // would leave the guard stuck forever and this second call would never happen. + const depPR2 = makePullRequest({ + id: 200, + nodeId: "PR_B", + repoFullName: "owner/repo2", + title: "Update dependency other-pkg", + userLogin: "dependabot[bot]", + headRef: "dependabot/npm_and_yarn/other-pkg", + }); + vi.mocked(pollService.fetchAllData).mockResolvedValue({ + issues: [], + pullRequests: [depPR1, depPR2], + workflowRuns: [], + errors: [], + }); + if (capturedFetchAll) { + await capturedFetchAll(); + } + + await vi.waitFor(() => { + expect(graphqlSpy).toHaveBeenCalledTimes(2); + }, { timeout: 5000 }); + } finally { + vi.useRealTimers(); + } + }); + + it("excludes a PR whose body-fetch failed from retry until the failure cooldown elapses", async () => { + // Only Date is faked (not setTimeout/setInterval) so we can jump the clock + // forward by the cooldown window without executing DashboardPage's other + // unrelated real timers/intervals along the way. + vi.useFakeTimers({ toFake: ["Date"] }); + try { + const githubService = await import("../../src/app/services/github"); + const graphqlSpy = vi.fn() + .mockRejectedValueOnce(new Error("boom")) + .mockResolvedValue({ nodes: [{ databaseId: 100, body: "not a renovate table" }], rateLimit: null }); + vi.mocked(githubService.getClient).mockReturnValue( + { graphql: graphqlSpy } as unknown as ReturnType + ); + + const depPR = makePullRequest({ + id: 100, + nodeId: "PR_A", + repoFullName: "owner/repo", + title: "Update dependency some-pkg", + userLogin: "dependabot[bot]", + headRef: "dependabot/npm_and_yarn/some-pkg", + }); + vi.mocked(pollService.fetchAllData).mockResolvedValue({ + issues: [], + pullRequests: [depPR], + workflowRuns: [], + errors: [], + }); + + render(() => ); + await waitFor(() => expect(graphqlSpy).toHaveBeenCalledTimes(1)); + // capturedFetchAll only awaits the poll fetch itself, not the effect's + // fire-and-forget async body-fetch — give the (immediately-rejecting, + // no real network delay) catch/finally chain a moment to actually + // settle and release the guard before triggering the next poll, or it + // races the in-flight fetch and gets silently skipped ("fetch already + // in flight"). + await new Promise((resolve) => setTimeout(resolve, 100)); + + // Second poll cycle, same PR unchanged — the cooldown should suppress the + // retry. Re-issue the mock with a fresh array (same PR object) so SolidJS's + // store sees a reference change and actually re-runs dependent effects — + // reusing the exact same array/response object across polls is a no-op + // for reactivity and would make this test pass for the wrong reason. + vi.mocked(pollService.fetchAllData).mockResolvedValue({ + issues: [], + pullRequests: [depPR], + workflowRuns: [], + errors: [], + }); + if (capturedFetchAll) await capturedFetchAll(); + // Real (unfaked) short delay to let the reactive effect chain settle + // before asserting the call count did NOT increase. + await new Promise((resolve) => setTimeout(resolve, 50)); + expect(graphqlSpy).toHaveBeenCalledTimes(1); + + // Jump the clock past the cooldown window (no intervening timers run — + // only Date.now() changes) so the PR becomes eligible again. + vi.setSystemTime(new Date(Date.now() + DEP_BODY_FAILURE_COOLDOWN_MS)); + vi.mocked(pollService.fetchAllData).mockResolvedValue({ + issues: [], + pullRequests: [depPR], + workflowRuns: [], + errors: [], + }); + if (capturedFetchAll) await capturedFetchAll(); + await waitFor(() => expect(graphqlSpy).toHaveBeenCalledTimes(2)); + } finally { + vi.useRealTimers(); + } + }); + + it("prunes a stale cooldown entry when its PR leaves the dependency set, so a later reappearance is retried immediately", async () => { + const githubService = await import("../../src/app/services/github"); + const graphqlSpy = vi.fn() + .mockRejectedValueOnce(new Error("boom")) + .mockResolvedValue({ nodes: [], rateLimit: null }); + vi.mocked(githubService.getClient).mockReturnValue( + { graphql: graphqlSpy } as unknown as ReturnType + ); + + const depPRA = makePullRequest({ + id: 100, + nodeId: "PR_A", + repoFullName: "owner/repo", + title: "Update dependency some-pkg", + userLogin: "dependabot[bot]", + headRef: "dependabot/npm_and_yarn/some-pkg", + }); + vi.mocked(pollService.fetchAllData).mockResolvedValue({ + issues: [], + pullRequests: [depPRA], + workflowRuns: [], + errors: [], + }); + + render(() => ); + await waitFor(() => expect(graphqlSpy).toHaveBeenCalledTimes(1)); + // capturedFetchAll only awaits the poll fetch itself, not the effect's + // fire-and-forget async body-fetch — give the (immediately-rejecting, + // no real network delay) catch/finally chain a moment to actually settle + // and release the guard before triggering the next poll, or it races the + // in-flight fetch and gets silently skipped ("fetch already in flight"). + await new Promise((resolve) => setTimeout(resolve, 100)); + + // PR_A closes/merges and disappears; a new PR_B needs its own fetch — + // this re-runs the effect (and its cooldown-prune step) even though + // PR_A itself is no longer in toFetch. + const depPRB = makePullRequest({ + id: 200, + nodeId: "PR_B", + repoFullName: "owner/repo2", + title: "Update dependency other-pkg", + userLogin: "dependabot[bot]", + headRef: "dependabot/npm_and_yarn/other-pkg", + }); + vi.mocked(pollService.fetchAllData).mockResolvedValue({ + issues: [], + pullRequests: [depPRB], + workflowRuns: [], + errors: [], + }); + if (capturedFetchAll) await capturedFetchAll(); + await waitFor(() => expect(graphqlSpy).toHaveBeenCalledTimes(2)); + await new Promise((resolve) => setTimeout(resolve, 100)); + + // PR_A reappears (e.g. reopened) well within what would have been its + // cooldown window. If its stale cooldown entry survived PR_A leaving the + // dependency set, it would incorrectly still be excluded here. + vi.mocked(pollService.fetchAllData).mockResolvedValue({ + issues: [], + pullRequests: [depPRA, depPRB], + workflowRuns: [], + errors: [], + }); + if (capturedFetchAll) await capturedFetchAll(); + await waitFor(() => expect(graphqlSpy).toHaveBeenCalledTimes(3)); + + const idsRequestedInThirdCall = (graphqlSpy.mock.calls[2]?.[1] as { ids: string[] } | undefined)?.ids ?? []; + expect(idsRequestedInThirdCall).toContain("PR_A"); + }); + + it("prunes a stale cooldown entry even when no other PR triggers a fetch in between", async () => { + // Regression guard: the cooldown-prune loop must run on every effect + // evaluation, not only when toFetch is non-empty — otherwise a PR that + // fails, disappears, and reopens with nothing else needing a fetch in + // between would stay incorrectly excluded on a stale cooldown entry. + const githubService = await import("../../src/app/services/github"); + const graphqlSpy = vi.fn() + .mockRejectedValueOnce(new Error("boom")) + .mockResolvedValue({ nodes: [], rateLimit: null }); + vi.mocked(githubService.getClient).mockReturnValue( + { graphql: graphqlSpy } as unknown as ReturnType + ); + + const depPRA = makePullRequest({ + id: 100, + nodeId: "PR_A", + repoFullName: "owner/repo", + title: "Update dependency some-pkg", + userLogin: "dependabot[bot]", + headRef: "dependabot/npm_and_yarn/some-pkg", + }); + vi.mocked(pollService.fetchAllData).mockResolvedValue({ + issues: [], + pullRequests: [depPRA], + workflowRuns: [], + errors: [], + }); + + render(() => ); + await waitFor(() => expect(graphqlSpy).toHaveBeenCalledTimes(1)); + await new Promise((resolve) => setTimeout(resolve, 100)); + + // PR_A closes/merges and disappears — no other dependency PR takes its + // place, so toFetch is empty this cycle. The prune step must still run. + vi.mocked(pollService.fetchAllData).mockResolvedValue({ + issues: [], + pullRequests: [], + workflowRuns: [], + errors: [], + }); + if (capturedFetchAll) await capturedFetchAll(); + await new Promise((resolve) => setTimeout(resolve, 100)); + expect(graphqlSpy).toHaveBeenCalledTimes(1); + + // PR_A reappears (e.g. reopened) well within what would have been its + // cooldown window. If the stale entry wasn't pruned while PR_A was absent, + // it would incorrectly still be excluded here. + vi.mocked(pollService.fetchAllData).mockResolvedValue({ + issues: [], + pullRequests: [depPRA], + workflowRuns: [], + errors: [], + }); + if (capturedFetchAll) await capturedFetchAll(); + await waitFor(() => expect(graphqlSpy).toHaveBeenCalledTimes(2)); + }); +}); + // ── Dependencies tab — abandonedDepsMap + dashboardIssueUrls reset on auth clear ─ describe("DashboardPage — abandonedDepsMap and dashboardIssueUrls on auth clear", () => { diff --git a/tests/services/api-dashboard-bodies.test.ts b/tests/services/api-dashboard-bodies.test.ts index 9a7f0fe3..bb7987e3 100644 --- a/tests/services/api-dashboard-bodies.test.ts +++ b/tests/services/api-dashboard-bodies.test.ts @@ -62,8 +62,9 @@ describe("fetchDashboardIssueBodies — empty input", () => { it("returns an empty Map immediately without calling graphql", async () => { const octokit = makeOctokit(async () => ({})); const result = await fetchDashboardIssueBodies(octokit, []); - expect(result).toBeInstanceOf(Map); - expect(result.size).toBe(0); + expect(result.bodies).toBeInstanceOf(Map); + expect(result.bodies.size).toBe(0); + expect(result.failedIds.size).toBe(0); expect(octokit.graphql).not.toHaveBeenCalled(); }); }); @@ -81,9 +82,9 @@ describe("fetchDashboardIssueBodies — single batch", () => { const result = await fetchDashboardIssueBodies(octokit, ["N_1", "N_2"]); - expect(result.get("N_1")).toBe("Dashboard body text"); - expect(result.get("N_2")).toBeNull(); - expect(result.size).toBe(2); + expect(result.bodies.get("N_1")).toBe("Dashboard body text"); + expect(result.bodies.get("N_2")).toBeNull(); + expect(result.bodies.size).toBe(2); }); it("calls updateGraphqlRateLimit when rateLimit is in response", async () => { @@ -116,8 +117,8 @@ describe("fetchDashboardIssueBodies — single batch", () => { const result = await fetchDashboardIssueBodies(octokit, ["N_1", "N_2", "N_3"]); - expect(result.size).toBe(1); - expect(result.get("N_2")).toBe("valid body"); + expect(result.bodies.size).toBe(1); + expect(result.bodies.get("N_2")).toBe("valid body"); }); }); @@ -134,7 +135,7 @@ describe("fetchDashboardIssueBodies — batch splitting", () => { const result = await fetchDashboardIssueBodies(octokit, ids); expect(octokit.graphql).toHaveBeenCalledTimes(2); - expect(result.size).toBe(NODES_BATCH_SIZE + 1); + expect(result.bodies.size).toBe(NODES_BATCH_SIZE + 1); }); it("first batch has exactly NODES_BATCH_SIZE items", async () => { @@ -166,9 +167,9 @@ describe("fetchDashboardIssueBodies — batch splitting", () => { const result = await fetchDashboardIssueBodies(octokit, allIds); - expect(result.size).toBe(NODES_BATCH_SIZE + 2); - expect(result.get("A_0")).toBe("body-A_0"); - expect(result.get("B_1")).toBe("body-B_1"); + expect(result.bodies.size).toBe(NODES_BATCH_SIZE + 2); + expect(result.bodies.get("A_0")).toBe("body-A_0"); + expect(result.bodies.get("B_1")).toBe("body-B_1"); }); }); @@ -184,8 +185,8 @@ describe("fetchDashboardIssueBodies — partial error handling", () => { const result = await fetchDashboardIssueBodies(octokit, ["N_null1", "N_ok", "N_null2"]); - expect(result.get("N_ok")).toBe("good body"); - expect(result.size).toBe(1); + expect(result.bodies.get("N_ok")).toBe("good body"); + expect(result.bodies.size).toBe(1); }); it("gracefully ignores nodes missing an id field", async () => { @@ -199,8 +200,8 @@ describe("fetchDashboardIssueBodies — partial error handling", () => { const result = await fetchDashboardIssueBodies(octokit, ["N_empty", "N_valid"]); // Node with empty id is skipped (falsy guard: !node.id) - expect(result.get("N_valid")).toBe("kept"); - expect(result.size).toBe(1); + expect(result.bodies.get("N_valid")).toBe("kept"); + expect(result.bodies.size).toBe(1); }); }); @@ -212,8 +213,9 @@ describe("fetchDashboardIssueBodies — GraphQL error handling", () => { const result = await fetchDashboardIssueBodies(octokit, ["N_1"]); - expect(result).toBeInstanceOf(Map); - expect(result.size).toBe(0); + expect(result.bodies).toBeInstanceOf(Map); + expect(result.bodies.size).toBe(0); + expect(result.failedIds.has("N_1")).toBe(true); }); it("calls updateGraphqlRateLimit from partial data on GraphQL error", async () => { @@ -251,8 +253,9 @@ describe("fetchDashboardIssueBodies — GraphQL error handling", () => { const result = await fetchDashboardIssueBodies(octokit, ids); // Second batch (1 item) should succeed despite first failing - expect(result.size).toBe(1); - expect(result.get(`N_${NODES_BATCH_SIZE}`)).toBe(`body-N_${NODES_BATCH_SIZE}`); + expect(result.bodies.size).toBe(1); + expect(result.bodies.get(`N_${NODES_BATCH_SIZE}`)).toBe(`body-N_${NODES_BATCH_SIZE}`); + expect(result.failedIds.size).toBe(NODES_BATCH_SIZE); }); }); @@ -268,8 +271,9 @@ describe("fetchDashboardIssueBodies — hung request timeout", () => { await vi.advanceTimersByTimeAsync(GRAPHQL_BODY_FETCH_TIMEOUT_MS); const result = await resultPromise; - expect(result).toBeInstanceOf(Map); - expect(result.size).toBe(0); + expect(result.bodies).toBeInstanceOf(Map); + expect(result.bodies.size).toBe(0); + expect(result.failedIds.has("N_1")).toBe(true); expect(warnSpy).toHaveBeenCalled(); expect(captureException).toHaveBeenCalledWith( expect.any(Error), diff --git a/tests/services/api-dep-pr-bodies.test.ts b/tests/services/api-dep-pr-bodies.test.ts index 978bb8cd..2f0c2ad1 100644 --- a/tests/services/api-dep-pr-bodies.test.ts +++ b/tests/services/api-dep-pr-bodies.test.ts @@ -54,8 +54,9 @@ describe("fetchDepPRBodies — empty input", () => { it("returns an empty Map without calling graphql", async () => { const octokit = makeOctokit(async () => ({})); const result = await fetchDepPRBodies(octokit, []); - expect(result).toBeInstanceOf(Map); - expect(result.size).toBe(0); + expect(result.bodies).toBeInstanceOf(Map); + expect(result.bodies.size).toBe(0); + expect(result.failedIds.size).toBe(0); expect(octokit.graphql).not.toHaveBeenCalled(); }); }); @@ -72,9 +73,9 @@ describe("fetchDepPRBodies — single batch", () => { const result = await fetchDepPRBodies(octokit, ["PR_node_1", "PR_node_2"]); - expect(result.size).toBe(2); - expect(result.get(42)).toContain("lodash"); - expect(result.get(99)).toBe("Some PR body"); + expect(result.bodies.size).toBe(2); + expect(result.bodies.get(42)).toContain("lodash"); + expect(result.bodies.get(99)).toBe("Some PR body"); }); it("skips nodes with null body", async () => { @@ -88,9 +89,9 @@ describe("fetchDepPRBodies — single batch", () => { const result = await fetchDepPRBodies(octokit, ["N_1", "N_2"]); - expect(result.size).toBe(1); - expect(result.get(1)).toBe("has body"); - expect(result.has(2)).toBe(false); + expect(result.bodies.size).toBe(1); + expect(result.bodies.get(1)).toBe("has body"); + expect(result.bodies.has(2)).toBe(false); }); it("skips nodes with empty string body", async () => { @@ -104,9 +105,9 @@ describe("fetchDepPRBodies — single batch", () => { const result = await fetchDepPRBodies(octokit, ["N_1", "N_2"]); - expect(result.size).toBe(1); - expect(result.has(1)).toBe(false); - expect(result.get(2)).toBe("content"); + expect(result.bodies.size).toBe(1); + expect(result.bodies.has(1)).toBe(false); + expect(result.bodies.get(2)).toBe("content"); }); it("skips null nodes in the response array", async () => { @@ -117,8 +118,8 @@ describe("fetchDepPRBodies — single batch", () => { const result = await fetchDepPRBodies(octokit, ["N_1", "N_2", "N_3"]); - expect(result.size).toBe(1); - expect(result.get(5)).toBe("valid"); + expect(result.bodies.size).toBe(1); + expect(result.bodies.get(5)).toBe("valid"); }); it("skips nodes with null databaseId", async () => { @@ -132,8 +133,8 @@ describe("fetchDepPRBodies — single batch", () => { const result = await fetchDepPRBodies(octokit, ["N_1", "N_2"]); - expect(result.size).toBe(1); - expect(result.get(10)).toBe("good"); + expect(result.bodies.size).toBe(1); + expect(result.bodies.get(10)).toBe("good"); }); it("updates graphql rate limit from response", async () => { @@ -183,8 +184,10 @@ describe("fetchDepPRBodies — error resilience", () => { ]; const result = await fetchDepPRBodies(octokit, ids); - expect(result.size).toBe(1); - expect(result.get(1)).toBe("batch1"); + expect(result.bodies.size).toBe(1); + expect(result.bodies.get(1)).toBe("batch1"); + expect(result.failedIds.has("batch2_0")).toBe(true); + expect(result.failedIds.size).toBe(1); }); it("updates rate limit from partial error response", async () => { @@ -213,8 +216,9 @@ describe("fetchDepPRBodies — hung request timeout", () => { await vi.advanceTimersByTimeAsync(GRAPHQL_BODY_FETCH_TIMEOUT_MS); const result = await resultPromise; - expect(result).toBeInstanceOf(Map); - expect(result.size).toBe(0); + expect(result.bodies).toBeInstanceOf(Map); + expect(result.bodies.size).toBe(0); + expect(result.failedIds.has("N_1")).toBe(true); expect(warnSpy).toHaveBeenCalled(); expect(captureException).toHaveBeenCalledWith( expect.any(Error), @@ -241,8 +245,9 @@ describe("fetchDepPRBodies — hung request timeout", () => { const resultPromise = fetchDepPRBodies(octokit, ["N_1"]); vi.mocked(getClient).mockReturnValue(null); await vi.advanceTimersByTimeAsync(GRAPHQL_BODY_FETCH_TIMEOUT_MS); - await resultPromise; + const result = await resultPromise; + expect(result.failedIds.has("N_1")).toBe(true); expect(warnSpy).toHaveBeenCalled(); expect(captureException).toHaveBeenCalled(); expect(pushNotification).not.toHaveBeenCalled();