diff --git a/github-code-search.ts b/github-code-search.ts index fb5fc84..902aac8 100644 --- a/github-code-search.ts +++ b/github-code-search.ts @@ -326,7 +326,15 @@ async function searchAction( ); } - const rawMatches = await fetchAllResults(effectiveQuery, org, GITHUB_TOKEN!, onRateLimit); + // Only retain raw file content when a regex filter is active — it's the + // only consumer (local fallback matching, issue #148) — see PR #154 review. + const rawMatches = await fetchAllResults( + effectiveQuery, + org, + GITHUB_TOKEN!, + onRateLimit, + regexFilter !== undefined, + ); let groups = aggregate( rawMatches, excludedRepos, diff --git a/src/aggregate.test.ts b/src/aggregate.test.ts index efb78c0..274d7bd 100644 --- a/src/aggregate.test.ts +++ b/src/aggregate.test.ts @@ -334,3 +334,137 @@ describe("aggregate — regexFilter", () => { expect(groups[0].matches[0].textMatches[0].fragment).toBe("import axios from 'axios'"); }); }); + +// ─── aggregate — fileContent fallback (issue #148) ──────────────────────────── + +describe("aggregate — regexFilter fallback to full fileContent", () => { + it("keeps a match whose API fragment does not contain the full regex match but whose fileContent does", () => { + // The API fragment only shows a narrower context (e.g. "react-dom" noise); + // the regex needs "react": "18 which only the full file content has. + const fileContent = + 'deps:\n "prettier": "latest",\n "react": "18.2.0",\n "react-dom": "18.2.0"\n'; + const matches: CodeMatch[] = [ + { + path: "package.json", + repoFullName: "myorg/repoA", + htmlUrl: "https://github.com/myorg/repoA/blob/main/package.json", + archived: false, + fileContent, + textMatches: [{ fragment: '"react-dom": "18.2.0"', matches: [] }], + }, + ]; + + const groups = aggregate(matches, new Set(), new Set(), false, /"react": "18/); + expect(groups).toHaveLength(1); + const tm = groups[0].matches[0].textMatches[0]; + expect(tm.fragment).toContain('"react": "18'); + expect(tm.matches[0].text).toBe('"react": "18'); + }); + + it("drops the match when neither the fragment nor fileContent contain a match", () => { + const matches: CodeMatch[] = [ + { + path: "package.json", + repoFullName: "myorg/repoA", + htmlUrl: "", + archived: false, + fileContent: 'deps:\n "vue": "3.0.0"\n', + textMatches: [{ fragment: '"vue": "3.0.0"', matches: [] }], + }, + ]; + + const groups = aggregate(matches, new Set(), new Set(), false, /"react": "18/); + expect(groups).toHaveLength(0); + }); + + it("does not use fileContent when the API fragment already matches (no behaviour change)", () => { + const fileContent = 'deps:\n "react": "18.2.0"\n'; + const matches: CodeMatch[] = [ + { + path: "package.json", + repoFullName: "myorg/repoA", + htmlUrl: "", + archived: false, + fileContent, + textMatches: [{ fragment: '"react": "18.2.0"', matches: [] }], + }, + ]; + + const groups = aggregate(matches, new Set(), new Set(), false, /"react": "18/); + expect(groups[0].matches[0].textMatches[0].fragment).toBe('"react": "18.2.0"'); + }); + + it("falls back to fragment-only behaviour (drops the match) when fileContent is absent", () => { + // Backward compatibility: no fileContent field at all (e.g. raw content + // fetch failed) behaves exactly as before this issue's fix. + const matches: CodeMatch[] = [ + { + path: "package.json", + repoFullName: "myorg/repoA", + htmlUrl: "", + archived: false, + textMatches: [{ fragment: '"react-dom": "18.2.0"', matches: [] }], + }, + ]; + + const groups = aggregate(matches, new Set(), new Set(), false, /"react": "18/); + expect(groups).toHaveLength(0); + }); + + it("keeps a match whose span reaches beyond the default ±2-line context window", () => { + // The match runs from "BEGIN" (line 11) to "END" (line 22) — far wider + // than a fixed window built only around the match's start line, which + // would never include "END" and silently drop an otherwise-valid match. + const decoysBefore = Array.from({ length: 10 }, (_, i) => `d${i + 1}`); + const body = Array.from({ length: 10 }, (_, i) => `m${i + 1}`); + const decoysAfter = Array.from({ length: 3 }, (_, i) => `d${i + 11}`); + const fileContent = [...decoysBefore, "BEGIN", ...body, "END", ...decoysAfter].join("\n"); + const matches: CodeMatch[] = [ + { + path: "file.txt", + repoFullName: "myorg/repoA", + htmlUrl: "", + archived: false, + fileContent, + textMatches: [{ fragment: "unrelated fragment", matches: [] }], + }, + ]; + + const groups = aggregate(matches, new Set(), new Set(), false, /BEGIN[\s\S]*?END/); + expect(groups).toHaveLength(1); + const tm = groups[0].matches[0].textMatches[0]; + expect(tm.matches[0].text).toBe(["BEGIN", ...body, "END"].join("\n")); + expect(tm.matches[0].line).toBe(11); + expect(tm.fragment).toContain("BEGIN"); + expect(tm.fragment).toContain("END"); + expect(tm.fragment).not.toContain("d1\n"); + }); + + it("finds every separate match rather than only the first when falling back to fileContent", () => { + const fileContent = [ + "match_A here", + "gap", + "gap", + "gap", + "gap", + "gap", + "gap", + "another match_A here", + ].join("\n"); + const matches: CodeMatch[] = [ + { + path: "file.txt", + repoFullName: "myorg/repoA", + htmlUrl: "", + archived: false, + fileContent, + textMatches: [{ fragment: "unrelated fragment", matches: [] }], + }, + ]; + + const groups = aggregate(matches, new Set(), new Set(), false, /match_A/); + const tms = groups[0].matches[0].textMatches; + expect(tms).toHaveLength(2); + expect(tms.map((tm) => tm.matches[0].line)).toEqual([1, 8]); + }); +}); diff --git a/src/aggregate.ts b/src/aggregate.ts index 4a73024..e75e419 100644 --- a/src/aggregate.ts +++ b/src/aggregate.ts @@ -79,6 +79,77 @@ function recomputeSegments( // ─── Aggregation ───────────────────────────────────────────────────────────── +/** + * Finds every match of `re` directly against the full downloaded `content` + * and builds a display-friendly fallback `TextMatch` per match, used when the + * API-provided fragment does not contain a match but the full file content + * does — see issue #148. + * + * Matches (and their line/col) are computed once against the *entire* file in + * a single forward pass — no rescanning of the preceding text per match, and + * no fixed-size window built only from the match's start line, so a match + * spanning more than `contextLines * 2 + 1` lines (or reached via + * lookaround) is never silently dropped: the window is expanded to always + * contain the match's full span. + */ +function fallbackMatchesFromFullContent( + content: string, + re: RegExp, + contextLines = 2, +): TextMatch[] { + re.lastIndex = 0; + // Precompute newline offsets once — O(n) — so line/col and window-boundary + // lookups are O(log n) via binary search instead of O(n) per match. + const newlines: number[] = []; + for (let i = 0; i < content.length; i++) { + if (content[i] === "\n") newlines.push(i); + } + const lineOfOffset = (offset: number): number => { + let lo = 0; + let hi = newlines.length; + while (lo < hi) { + const mid = (lo + hi) >> 1; + if (newlines[mid] < offset) lo = mid + 1; + else hi = mid; + } + return lo + 1; // 1-based + }; + const startOffsetOfLine = (line: number): number => (line === 1 ? 0 : newlines[line - 2] + 1); + const totalLines = newlines.length + 1; + const lines = content.split("\n"); + + const results: TextMatch[] = []; + let m: RegExpExecArray | null; + while ((m = re.exec(content)) !== null) { + const start = m.index; + const end = start + m[0].length; + const startLine = lineOfOffset(start); + // `end - 1` so a match ending exactly on a newline still resolves to the + // line it belongs to rather than the following (empty) one. + const endLine = end > start ? lineOfOffset(end - 1) : startLine; + + const windowStartLine = Math.max(1, startLine - contextLines); + const windowEndLine = Math.min(totalLines, endLine + contextLines); + const windowStartOffset = startOffsetOfLine(windowStartLine); + const fragment = lines.slice(windowStartLine - 1, windowEndLine).join("\n"); + const col = start - startOffsetOfLine(startLine) + 1; + + results.push({ + fragment, + matches: [ + { + text: m[0], + indices: [start - windowStartOffset, end - windowStartOffset], + line: startLine, + col, + }, + ], + }); + if (m[0].length === 0) re.lastIndex++; // guard against zero-width matches + } + return results; +} + export function aggregate( matches: CodeMatch[], excludedRepos: Set, @@ -107,7 +178,7 @@ export function aggregate( // Preserve the caller's lastIndex: aggregate() must not have observable // side-effects on the passed-in RegExp instance. const savedLastIndex = regexFilter!.lastIndex; - const updatedTextMatches: TextMatch[] = m.textMatches + let updatedTextMatches: TextMatch[] = m.textMatches .map((tm) => { // Derive the absolute start line of this fragment from the first API // segment. If no API segment is available, fall back to 1 so that @@ -124,6 +195,13 @@ export function aggregate( return segs.length > 0 ? { fragment: tm.fragment, matches: segs } : null; }) .filter((tm): tm is TextMatch => tm !== null); + // Fix: fall back to the full downloaded file content when none of the + // API-provided fragments contain a match — the API fragment can be too + // narrow to include the whole regex match even though the file does + // contain it, silently dropping otherwise-valid results — see issue #148. + if (updatedTextMatches.length === 0 && m.fileContent) { + updatedTextMatches = fallbackMatchesFromFullContent(m.fileContent, globalRe); + } // Restore the caller's original lastIndex (rather than hard-coding 0), // so aggregate() doesn't have observable side effects on its inputs. regexFilter!.lastIndex = savedLastIndex; diff --git a/src/api.test.ts b/src/api.test.ts index 83ae3bb..c46791b 100644 --- a/src/api.test.ts +++ b/src/api.test.ts @@ -338,6 +338,75 @@ describe("fetchAllResults", () => { const results = await fetchAllResults("hello", "org", "tok"); expect(results[0].textMatches[0].matches[0].line).toBe(1); }); + + it("propagates the downloaded raw content into fileContent when keepFileContent is true", async () => { + const fileContent = "line one\nline two\nconst x = doSomething()\n"; + const fakeItem = { + path: "src/mod.ts", + html_url: "https://github.com/org/repo/blob/main/src/mod.ts", + repository: { full_name: "org/repo", archived: false }, + text_matches: [{ fragment: "const x = doSomething()", matches: [] }], + }; + globalThis.fetch = (async (url: string | URL | Request) => { + const urlStr = url.toString(); + if (new URL(urlStr).hostname === "raw.githubusercontent.com") { + return new Response(fileContent, { status: 200 }); + } + return new Response(JSON.stringify({ items: [fakeItem], total_count: 1 }), { + status: 200, + headers: { "content-type": "application/json" }, + }); + }) as typeof fetch; + + const results = await fetchAllResults("doSomething", "org", "tok", undefined, true); + expect(results[0].fileContent).toBe(fileContent); + }); + + it("leaves fileContent absent when the raw content fetch fails, even with keepFileContent true", async () => { + const fakeItem = { + path: "src/mod.ts", + html_url: "https://github.com/org/repo/blob/main/src/mod.ts", + repository: { full_name: "org/repo", archived: false }, + text_matches: [{ fragment: "hello world", matches: [] }], + }; + globalThis.fetch = (async (url: string | URL | Request) => { + const urlStr = url.toString(); + if (new URL(urlStr).hostname === "raw.githubusercontent.com") { + return new Response("Not Found", { status: 404 }); + } + return new Response(JSON.stringify({ items: [fakeItem], total_count: 1 }), { + status: 200, + headers: { "content-type": "application/json" }, + }); + }) as typeof fetch; + + const results = await fetchAllResults("hello", "org", "tok", undefined, true); + expect(results[0].fileContent).toBeUndefined(); + }); + + it("does not retain fileContent when keepFileContent is false (default — non-regex queries)", async () => { + const fileContent = "line one\nline two\nconst x = doSomething()\n"; + const fakeItem = { + path: "src/mod.ts", + html_url: "https://github.com/org/repo/blob/main/src/mod.ts", + repository: { full_name: "org/repo", archived: false }, + text_matches: [{ fragment: "const x = doSomething()", matches: [] }], + }; + globalThis.fetch = (async (url: string | URL | Request) => { + const urlStr = url.toString(); + if (new URL(urlStr).hostname === "raw.githubusercontent.com") { + return new Response(fileContent, { status: 200 }); + } + return new Response(JSON.stringify({ items: [fakeItem], total_count: 1 }), { + status: 200, + headers: { "content-type": "application/json" }, + }); + }) as typeof fetch; + + // No keepFileContent argument — defaults to false. + const results = await fetchAllResults("doSomething", "org", "tok"); + expect(results[0].fileContent).toBeUndefined(); + }); }); // ─── fetchRepoTeams ────────────────────────────────────────────────────────── diff --git a/src/api.ts b/src/api.ts index ab7e2b0..693e435 100644 --- a/src/api.ts +++ b/src/api.ts @@ -196,6 +196,10 @@ export async function fetchAllResults( org: string, token: string, onRateLimit?: (waitMs: number) => Promise, + // Only regex-mode local filtering (issue #148) ever reads `fileContent`. + // Default to false so ordinary (non-regex) queries don't keep every + // downloaded raw file alive in memory for the whole run — see PR #154 review. + keepFileContent = false, ): Promise { // Write the initial progress line (no newline — will be overwritten by \r). process.stderr.write(pc.dim(" Fetching results from GitHub…")); @@ -278,6 +282,11 @@ export async function fetchAllResults( htmlUrl: item.html_url, archived: item.repository.archived === true, isTemplate: item.repository.is_template === true, + // Propagate the already-downloaded content for local regex filtering + // fallback — see issue #148. No extra network calls: reuses the content + // fetched above for line-number resolution. Opt-in via `keepFileContent` + // so non-regex queries don't retain every raw file in memory. + fileContent: keepFileContent ? fileContent : undefined, textMatches: (item.text_matches ?? []).map((m) => { const fragment: string = m.fragment ?? ""; const fragmentStartLine = fileContent ? computeFragmentStartLine(fileContent, fragment) : 1; diff --git a/src/types.ts b/src/types.ts index 8e95aef..ae5f692 100644 --- a/src/types.ts +++ b/src/types.ts @@ -22,6 +22,11 @@ export interface CodeMatch { textMatches: TextMatch[]; archived: boolean; isTemplate?: boolean; + /** Full raw file content, when already downloaded by `fetchAllResults()` for + * line-number resolution. Used as a fallback for local regex filtering + * when the API-provided fragment does not contain the full match — see + * issue #148. Absent when the raw content could not be fetched. */ + fileContent?: string; } export interface RepoGroup {