diff --git a/github-code-search.ts b/github-code-search.ts index a6f583a..24733ba 100644 --- a/github-code-search.ts +++ b/github-code-search.ts @@ -35,6 +35,7 @@ import { checkForUpdate } from "./src/upgrade.ts"; import { runInteractive } from "./src/tui.ts"; import { generateCompletion, detectShell } from "./src/completions.ts"; import { getGhAuthToken } from "./src/gh-cli.ts"; +import { clipToWidth } from "./src/render.ts"; import { buildApiQuery, detectPathWildcardLimitation, @@ -363,18 +364,19 @@ async function searchAction( // No countdown running — start a fresh one. cooldownUntil = desiredEnd; activeCooldown = (async () => { - // Start on a fresh line so the countdown doesn't overwrite the progress bar - process.stderr.write("\n"); + // The fetch progress is paused and cleared while this countdown owns the row. while (true) { const remaining = cooldownUntil - Date.now(); if (remaining <= 0) break; process.stderr.write( - `\r ${style.yellow("Rate limited")} — resuming in ${formatRetryWait(remaining)}\u2026${" ".repeat(10)}`, + `\x1b[2K\r${clipToWidth( + ` ${style.yellow("Rate limited")} — resuming in ${formatRetryWait(remaining)}\u2026`, + Math.max(0, (process.stderr.columns ?? 80) - 1), + )}`, ); await new Promise((r) => setTimeout(r, 1_000)); } - // Leave cursor at line start; the next \r progress update will overwrite cleanly - process.stderr.write(`\r ${style.dim("Rate limited")} — resuming\u2026${" ".repeat(40)}`); + process.stderr.write("\x1b[2K\r"); })().finally(() => { activeCooldown = null; cooldownUntil = 0; diff --git a/src/api.test.ts b/src/api.test.ts index 7497d93..2b59064 100644 --- a/src/api.test.ts +++ b/src/api.test.ts @@ -1,5 +1,6 @@ -import { afterEach, beforeEach, describe, expect, it } from "bun:test"; +import { afterEach, beforeEach, describe, expect, it, spyOn } from "bun:test"; import { mockFetch } from "./test-setup.ts"; +import { visibleWidth } from "./render.ts"; import { buildFetchProgress, buildLineResolutionProgress, @@ -118,6 +119,237 @@ const makeFetchItem = (i: number) => ({ text_matches: [], }); +describe("fetch progress lifecycle (issues #247 and #248)", () => { + let writes: string[]; + let stderr: ReturnType>; + const columnsDescriptor = Object.getOwnPropertyDescriptor(process.stderr, "columns"); + + beforeEach(() => { + writes = []; + Object.defineProperty(process.stderr, "columns", { + configurable: true, + value: 200, + writable: true, + }); + stderr = spyOn(process.stderr, "write").mockImplementation((chunk) => { + writes.push(String(chunk)); + return true; + }); + }); + + afterEach(() => { + stderr.mockRestore(); + globalThis.fetch = originalFetch; + if (columnsDescriptor) Object.defineProperty(process.stderr, "columns", columnsDescriptor); + else Reflect.deleteProperty(process.stderr, "columns"); + }); + + it.each([0, 1])("clears the results progress line after fetching %i results", async (count) => { + mockFetch(async () => + Response.json({ + items: Array.from({ length: count }, (_, i) => makeFetchItem(i)), + total_count: count, + }), + ); + + await fetchAllResults("q", "org", "tok"); + + expect(writes.at(-1)).toBe("\x1b[2K\r"); + expect(writes).not.toContain("\n"); + }); + + it("clears the results progress line before propagating an API error", async () => { + mockFetch(async () => Response.json({ message: "Unauthorized" }, { status: 401 })); + + await expect(fetchAllResults("q", "org", "tok")).rejects.toThrow("401"); + + expect(writes.at(-1)).toBe("\x1b[2K\r"); + }); + + it("clears results before line resolution and clears resolution when finished", async () => { + mockFetch(async (url) => + new URL(String(url)).hostname === "raw.githubusercontent.com" + ? new Response("hello") + : Response.json({ + items: [{ ...makeFetchItem(0), text_matches: [{ fragment: "hello" }] }], + total_count: 1, + }), + ); + + await fetchAllResults("hello", "org", "tok"); + + const resolutionStart = writes.findIndex((line) => line.includes("Resolving line numbers")); + expect(writes[resolutionStart - 1]).toBe("\x1b[2K\r"); + expect(writes.at(-1)).toBe("\x1b[2K\r"); + }); + + it.each([0, 1, 2])("shows matching progress style for %i teams and clears it", async (count) => { + mockFetch(async (url) => + String(url).includes("/teams?") + ? Response.json( + Array.from({ length: count }, (_, i) => ({ slug: `squad-${i}`, name: `Team ${i}` })), + ) + : new Response(null, { status: 404 }), + ); + + await fetchRepoTeams("org", "tok", ["squad-", "chapter-"], false); + + const label = `Fetching repos for ${count} team${count !== 1 ? "s" : ""} matching prefixes [squad-, chapter-]…`; + const expected = (done: number) => + buildLineResolutionProgress(done, count).replace("Resolving line numbers…", label); + expect(writes).toEqual([ + expected(0), + ...Array.from({ length: count }, (_, i) => expected(i + 1)), + "\x1b[2K\r", + ]); + }); + + it("counts a team only after all of its repository pages finish", async () => { + mockFetch(async (url) => { + const parsed = new URL(String(url)); + if (parsed.pathname.endsWith("/teams")) { + return Response.json([{ slug: "squad-web", name: "Web" }]); + } + expect(writes.at(-1)).toContain("0/1"); + return Response.json( + parsed.searchParams.get("page") === "1" + ? Array.from({ length: 100 }, (_, i) => ({ full_name: `org/repo-${i}` })) + : [], + ); + }); + + const teams = await fetchRepoTeams("org", "tok", ["squad-"], false); + + expect(teams.size).toBe(100); + expect(writes.at(-2)).toContain("1/1"); + expect(writes.at(-1)).toBe("\x1b[2K\r"); + }); + + it("clears team progress before propagating a repository fetch error", async () => { + mockFetch(async (url) => + String(url).includes("/teams?") + ? Response.json([{ slug: "squad-web", name: "Web" }]) + : Response.json({ message: "Unauthorized" }, { status: 401 }), + ); + + await expect(fetchRepoTeams("org", "tok", ["squad-"], false)).rejects.toThrow("401"); + + expect(writes.at(-1)).toBe("\x1b[2K\r"); + }); + + it.each(["results", "teams"])( + "clears %s progress before a countdown writes a newline", + async (phase) => { + let limited = false; + mockFetch(async (url) => { + if (phase === "teams" && new URL(String(url)).pathname.endsWith("/teams")) { + return Response.json([{ slug: "squad-web", name: "Web" }]); + } + if (!limited) { + limited = true; + return new Response(null, { status: 429, headers: { "Retry-After": "11" } }); + } + return phase === "results" + ? Response.json({ items: [], total_count: 0 }) + : Response.json([]); + }); + const countdown = async () => { + expect(writes.at(-1)).toBe("\x1b[2K\r"); + process.stderr.write("\n"); + }; + + if (phase === "results") await fetchAllResults("q", "org", "tok", countdown); + else await fetchRepoTeams("org", "tok", ["squad-"], false, countdown); + + expect(writes.at(-1)).toBe("\x1b[2K\r"); + }, + ); + + it.each(["results", "teams"])("clears %s progress when a countdown rejects", async (phase) => { + mockFetch(async (url) => { + if (phase === "teams" && new URL(String(url)).pathname.endsWith("/teams")) { + return Response.json([{ slug: "squad-web", name: "Web" }]); + } + return new Response(null, { status: 429, headers: { "Retry-After": "11" } }); + }); + const countdown = async () => { + expect(writes.at(-1)).toBe("\x1b[2K\r"); + process.stderr.write("\n"); + throw new Error("Countdown cancelled"); + }; + + await expect( + phase === "results" + ? fetchAllResults("q", "org", "tok", countdown) + : fetchRepoTeams("org", "tok", ["squad-"], false, countdown), + ).rejects.toThrow("Countdown cancelled"); + expect(writes.at(-1)).toBe("\x1b[2K\r"); + }); + + it("keeps all 93-team progress updates on one row at 80 columns", async () => { + process.stderr.columns = 80; + mockFetch(async (url) => + new URL(String(url)).pathname.endsWith("/teams") + ? Response.json( + Array.from({ length: 93 }, (_, i) => ({ slug: `squad-${i}`, name: `Team ${i}` })), + ) + : Response.json([]), + ); + + await fetchRepoTeams("org", "tok", ["squad-", "chapter-"], false); + + for (const line of writes.slice(0, -1)) { + expect(visibleWidth(line)).toBeLessThan(80); + expect(line).toContain("\x1b[38;5;129m"); + expect(line).toMatch(/\d+\/93$/); + } + expect(writes.at(-2)).toContain("93/93"); + expect(writes.at(-1)).toBe("\x1b[2K\r"); + }); + + it("does not redraw team progress while another team is showing a countdown", async () => { + let releaseCountdown!: () => void; + let signalCountdown!: () => void; + const countdownStarted = new Promise((resolve) => { + signalCountdown = resolve; + }); + const countdownFinished = new Promise((resolve) => { + releaseCountdown = resolve; + }); + let limited = false; + mockFetch(async (url) => { + const path = new URL(String(url)).pathname; + if (path.endsWith("/teams")) { + return Response.json([ + { slug: "squad-limited", name: "Limited" }, + { slug: "squad-fast", name: "Fast" }, + ]); + } + if (path.endsWith("/squad-limited/repos") && !limited) { + limited = true; + return new Response(null, { status: 429, headers: { "Retry-After": "11" } }); + } + if (path.endsWith("/squad-fast/repos")) await countdownStarted; + return Response.json([]); + }); + const fetch = fetchRepoTeams("org", "tok", ["squad-"], false, async () => { + process.stderr.write("\nCOUNTDOWN"); + signalCountdown(); + await countdownFinished; + }); + await countdownStarted; + // Let the other worker finish while the countdown still owns stderr. + await new Promise((resolve) => setTimeout(resolve, 0)); + const lastDuringCountdown = writes.at(-1); + releaseCountdown(); + await fetch; + + expect(lastDuringCountdown).toBe("\nCOUNTDOWN"); + expect(writes.at(-2)).toContain("2/2"); + expect(writes.at(-1)).toBe("\x1b[2K\r"); + }); +}); + describe("fetchAllResults", () => { beforeEach(() => { // biome-ignore lint/suspicious/noExplicitAny: test-only shim @@ -708,6 +940,15 @@ describe("buildFetchProgress", () => { // ─── buildLineResolutionProgress ───────────────────────────────────────────────── describe("buildLineResolutionProgress", () => { + it("preserves the full counter when the label budget is zero", () => { + const progress = buildLineResolutionProgress(93, 93, 31); + + expect(visibleWidth(progress)).toBe(30); + expect(progress).toEndWith("93/93"); + expect(progress).not.toContain("…"); + expect(progress.match(/▓/g)).toHaveLength(20); + }); + it("starts with \\r to overwrite the current line", () => { const s = buildLineResolutionProgress(5, 20); expect(s.startsWith("\r")).toBe(true); diff --git a/src/api.ts b/src/api.ts index 3491c0e..133c4eb 100644 --- a/src/api.ts +++ b/src/api.ts @@ -2,6 +2,7 @@ import * as style from "./style.ts"; import type { CodeMatch } from "./types.ts"; import { concurrentMap, fetchWithRetry, formatRetryWait, paginatedFetch } from "./api-utils.ts"; import { getCacheKey, readCache, writeCache } from "./cache.ts"; +import { clipToWidth, visibleWidth } from "./render.ts"; // ─── Raw GitHub API types (internal) ───────────────────────────────────────── @@ -187,9 +188,75 @@ function buildProgressBar(filled: number): string { * * Pure function — no side effects; exported for unit testing. */ -export function buildFetchProgress(currentPage: number, totalPages: number): string { +export function buildFetchProgress( + currentPage: number, + totalPages: number, + columns = Infinity, +): string { const filled = totalPages > 0 ? Math.round((currentPage / totalPages) * PROGRESS_BAR_WIDTH) : 0; - return `\r Fetching results from GitHub… ${buildProgressBar(filled)} page ${currentPage}/${totalPages}`; + return buildProgressLine( + "Fetching results from GitHub…", + `page ${currentPage}/${totalPages}`, + filled, + columns, + ); +} + +function buildProgressLine( + label: string, + counter: string, + filled: number, + columns: number, +): string { + const width = Math.max(0, columns - 1); + const suffix = ` ${buildProgressBar(filled)} ${counter}`; + const labelWidth = Math.max(0, width - visibleWidth(suffix) - 2); + const shortened = + labelWidth === 0 + ? "" + : visibleWidth(label) > labelWidth + ? `${clipToWidth(label, labelWidth - 1)}…` + : label; + return `\r${clipToWidth(` ${shortened}${suffix}`, width)}`; +} + +function buildCountProgress( + label: string, + done: number, + total: number, + columns = Infinity, +): string { + const filled = total > 0 ? Math.round((done / total) * PROGRESS_BAR_WIDTH) : 0; + return buildProgressLine(label, `${done}/${total}`, filled, columns); +} + +function createFetchProgress(onRateLimit?: (waitMs: number) => Promise) { + let latest = ""; + let waiting = 0; + const clear = () => { + process.stderr.write("\x1b[2K\r"); + }; + const write = (line: string) => { + latest = line; + if (waiting === 0) process.stderr.write(line); + }; + return { + write, + clear, + onRateLimit: onRateLimit + ? async (waitMs: number) => { + if (waiting++ === 0) clear(); + try { + await onRateLimit(waitMs); + } finally { + if (--waiting === 0) { + clear(); + if (latest) process.stderr.write(latest); + } + } + } + : undefined, + }; } /** @@ -198,9 +265,12 @@ export function buildFetchProgress(currentPage: number, totalPages: number): str * * Pure function — no side effects; exported for unit testing. */ -export function buildLineResolutionProgress(done: number, total: number): string { - const filled = total > 0 ? Math.round((done / total) * PROGRESS_BAR_WIDTH) : 0; - return `\r Resolving line numbers… ${buildProgressBar(filled)} ${done}/${total}`; +export function buildLineResolutionProgress( + done: number, + total: number, + columns = Infinity, +): string { + return buildCountProgress("Resolving line numbers…", done, total, columns); } export async function fetchAllResults( @@ -214,7 +284,13 @@ export async function fetchAllResults( keepFileContent = false, ): Promise { // Write the initial progress line (no newline — will be overwritten by \r). - process.stderr.write(style.dim(" Fetching results from GitHub…")); + const progress = createFetchProgress(onRateLimit); + progress.write( + clipToWidth( + style.dim(" Fetching results from GitHub…"), + Math.max(0, (process.stderr.columns ?? 80) - 1), + ), + ); let totalPages = 0; // GitHub code search is capped at 1000 results; paginatedFetch stops naturally // when a page returns fewer than 100 items. When total_count is an exact @@ -226,22 +302,23 @@ export async function fetchAllResults( // Short-circuit: GitHub caps at 1 000 results (10 pages max). // Once we know totalPages, skip any page that would exceed the cap. if (totalPages > 0 && page > totalPages) return []; - const { items, total } = await searchCode(query, org, token, page, onRateLimit); + const { items, total } = await searchCode(query, org, token, page, progress.onRateLimit); // On the first page, compute the expected number of pages so the bar // can show realistic progress. GitHub caps at 1 000 results (10 pages). if (page === 1) { totalPages = Math.min(Math.ceil(total / 100), 10); } if (totalPages > 0) { - process.stderr.write(buildFetchProgress(page, totalPages)); + progress.write(buildFetchProgress(page, totalPages, process.stderr.columns ?? 80)); } return items; }, 100, // GitHub returns up to 100 items per page 250, // Be kind to rate limits between pages - ); - // Terminate the progress line. - process.stderr.write("\n"); + ).finally(() => { + // Erase rather than terminate the transient line, including on failure (#247). + progress.clear(); + }); // ─── Resolve absolute line numbers ────────────────────────────────────── // The GitHub Code Search API returns fragment-relative character indices @@ -260,7 +337,9 @@ export async function fetchAllResults( // Fix: limit concurrency to avoid saturating the connection pool on slow // networks. A per-request 5 s timeout prevents indefinite stalls; // failures fall back to fragment-relative line numbers silently. — see issue #99 - process.stderr.write(buildLineResolutionProgress(0, urlsToFetch.length)); + progress.write( + buildLineResolutionProgress(0, urlsToFetch.length, process.stderr.columns ?? 80), + ); let resolved = 0; await concurrentMap( urlsToFetch, @@ -278,12 +357,14 @@ export async function fetchAllResults( // Fall back to fragment-relative line numbers (timeout or network error) } resolved++; - process.stderr.write(buildLineResolutionProgress(resolved, urlsToFetch.length)); + progress.write( + buildLineResolutionProgress(resolved, urlsToFetch.length, process.stderr.columns ?? 80), + ); }, { concurrency: 20 }, ); // Clear the resolution progress line before continuing (ANSI erase-entire-line). - process.stderr.write("\x1b[2K\r"); + progress.clear(); } return allItems.map((item) => { @@ -374,47 +455,53 @@ export async function fetchRepoTeams( } } - process.stderr.write( - style.dim( - `Fetching repos for ${matchingTeamSlugs.length} team${matchingTeamSlugs.length !== 1 ? "s" : ""} matching prefix${prefixes.length !== 1 ? "es" : ""} [${prefixes.join(", ")}]…\n`, - ), - ); + const totalTeams = matchingTeamSlugs.length; + const label = `Fetching repos for ${totalTeams} team${totalTeams !== 1 ? "s" : ""} matching prefix${prefixes.length !== 1 ? "es" : ""} [${prefixes.join(", ")}]…`; + const progress = createFetchProgress(onRateLimit); + progress.write(buildCountProgress(label, 0, totalTeams, process.stderr.columns ?? 80)); // ── 2. For each matching team fetch its repos (paginated) ────────────────── // @see https://docs.github.com/en/rest/teams/teams?apiVersion=2022-11-28#list-repos-in-a-team const repoTeams = new Map(); - - await Promise.all( - matchingTeamSlugs.map(async (slug) => { - // Paginate repos per-page and update repoTeams incrementally to avoid - // holding an extra full repos array in memory for large teams. - let reposPage = 1; - while (true) { - const params = new URLSearchParams({ per_page: "100", page: String(reposPage) }); - const res = await fetchWithRetry( - `https://api.github.com/orgs/${org}/teams/${slug}/repos?${params}`, - { headers: githubHeaders(token, "application/vnd.github+json") }, - 3, - onRateLimit, - ); - if (!res.ok) { - // 404 is expected for nested/secret teams — skip silently. - if (res.status === 404) break; - // All other errors (rate-limit included) delegate to throwApiError - // which detects rate-limits and produces a clean message. - await throwApiError(res); - } - const repos = (await res.json()) as RawRepo[]; - for (const r of repos) { - const list = repoTeams.get(r.full_name) ?? []; - if (!list.includes(slug)) list.push(slug); - repoTeams.set(r.full_name, list); - } - if (repos.length < 100) break; - reposPage++; + let completedTeams = 0; + + // Wait for every worker before clearing, so late completions cannot redraw + // the progress line after another team's request fails. + await concurrentMap(matchingTeamSlugs, async (slug) => { + // Paginate repos per-page and update repoTeams incrementally to avoid + // holding an extra full repos array in memory for large teams. + let reposPage = 1; + while (true) { + const params = new URLSearchParams({ per_page: "100", page: String(reposPage) }); + const res = await fetchWithRetry( + `https://api.github.com/orgs/${org}/teams/${slug}/repos?${params}`, + { headers: githubHeaders(token, "application/vnd.github+json") }, + 3, + progress.onRateLimit, + ); + if (!res.ok) { + // 404 is expected for nested/secret teams — skip silently. + if (res.status === 404) break; + // All other errors (rate-limit included) delegate to throwApiError + // which detects rate-limits and produces a clean message. + await throwApiError(res); } - }), - ); + const repos = (await res.json()) as RawRepo[]; + for (const r of repos) { + const list = repoTeams.get(r.full_name) ?? []; + if (!list.includes(slug)) list.push(slug); + repoTeams.set(r.full_name, list); + } + if (repos.length < 100) break; + reposPage++; + } + completedTeams++; + progress.write( + buildCountProgress(label, completedTeams, totalTeams, process.stderr.columns ?? 80), + ); + }).finally(() => { + progress.clear(); + }); // ── Persist result ────────────────────────────────────────────────────────── // Serialise the Map as an array of entries for JSON round-trip stability. diff --git a/src/render.ts b/src/render.ts index 5719edc..7a489d7 100644 --- a/src/render.ts +++ b/src/render.ts @@ -32,6 +32,7 @@ export { buildSelectionSummary, } from "./render/summary.ts"; export { applySelectAll, applySelectNone } from "./render/selection.ts"; +export { clipToWidth, visibleWidth } from "./render/terminal.ts"; // ─── buildFileUrl ─────────────────────────────────────────────────────────────