From a818982ea1140c303a080141af1507e71816d54f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20HOUZ=C3=89?= Date: Sun, 23 Aug 2026 14:16:42 +0200 Subject: [PATCH 1/2] Fall back to full file content for local regex filtering (#148) aggregate() previously tested the local regex filter only against TextMatch.fragment, the truncated excerpt returned by the GitHub API. If that excerpt did not cover the full portion matched by the regex, the repository was silently dropped even though the actual file matched. fetchAllResults() already downloads the raw file content from raw.githubusercontent.com to resolve absolute line numbers; it now propagates that content into CodeMatch.fileContent (src/types.ts) instead of discarding it. aggregate() falls back to matching against fileContent, and builds a small context window around each match for display, only when none of the API-provided fragments matched. No new network calls, and behaviour is unchanged when fileContent is absent or when the fragment already matches. Closes #148 --- src/aggregate.test.ts | 77 +++++++++++++++++++++++++++++++++++++++++++ src/aggregate.ts | 53 ++++++++++++++++++++++++++++- src/api.ts | 4 +++ src/types.ts | 5 +++ 4 files changed, 138 insertions(+), 1 deletion(-) diff --git a/src/aggregate.test.ts b/src/aggregate.test.ts index efb78c0..a125cd5 100644 --- a/src/aggregate.test.ts +++ b/src/aggregate.test.ts @@ -334,3 +334,80 @@ 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); + }); +}); diff --git a/src/aggregate.ts b/src/aggregate.ts index 4a73024..6a17619 100644 --- a/src/aggregate.ts +++ b/src/aggregate.ts @@ -79,6 +79,43 @@ function recomputeSegments( // ─── Aggregation ───────────────────────────────────────────────────────────── +/** + * Extracts a small multi-line window of `content` around the 1-based line + * `matchLine`, mirroring the size of context GitHub's own fragment field + * typically provides. Used to build a display-friendly fallback `TextMatch` + * when the API-provided fragment does not contain a match but the full + * downloaded file content does — see issue #148. + */ +function sliceContextWindow( + content: string, + matchLine: number, + contextLines = 2, +): { fragment: string; fragmentStartLine: number } { + const lines = content.split("\n"); + const startLine = Math.max(1, matchLine - contextLines); + const endLine = Math.min(lines.length, matchLine + contextLines); + return { + fragment: lines.slice(startLine - 1, endLine).join("\n"), + fragmentStartLine: startLine, + }; +} + +/** + * Finds the 1-based line numbers where `re` matches within `content`. + * `re` must be a global RegExp; its `lastIndex` is reset before use. + */ +function findMatchLines(content: string, re: RegExp): Set { + re.lastIndex = 0; + const lines = new Set(); + let m: RegExpExecArray | null; + while ((m = re.exec(content)) !== null) { + const before = content.slice(0, m.index); + lines.add((before.match(/\n/g)?.length ?? 0) + 1); + if (m[0].length === 0) re.lastIndex++; + } + return lines; +} + export function aggregate( matches: CodeMatch[], excludedRepos: Set, @@ -107,7 +144,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 +161,20 @@ 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) { + const fileContent = m.fileContent; + updatedTextMatches = [...findMatchLines(fileContent, globalRe)] + .map((matchLine) => { + const { fragment, fragmentStartLine } = sliceContextWindow(fileContent, matchLine); + const segs = recomputeSegments(fragment, globalRe, fragmentStartLine); + return segs.length > 0 ? { fragment, matches: segs } : null; + }) + .filter((tm): tm is TextMatch => tm !== null); + } // 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.ts b/src/api.ts index ab7e2b0..60ac874 100644 --- a/src/api.ts +++ b/src/api.ts @@ -278,6 +278,10 @@ 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. + fileContent, 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 { From e2af9022eca2932e1983edca9dc51dfe62c7fd91 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20HOUZ=C3=89?= Date: Sun, 23 Aug 2026 16:50:52 +0200 Subject: [PATCH 2/2] Address Copilot review on PR #154 (issue #148) - aggregate.ts: replace the O(n*m) prefix-rescan findMatchLines/sliceContextWindow pair with fallbackMatchesFromFullContent, which finds every match in a single forward pass over the full file content and expands the display window to always contain the match's full span, so matches longer than the +/-2 line window (or reached via lookaround) are no longer silently dropped. - api.ts: fetchAllResults now only retains raw fileContent on CodeMatch when the new keepFileContent flag is set, avoiding keeping every downloaded file in memory for ordinary (non-regex) queries. - github-code-search.ts: pass keepFileContent=true only when a regex filter is active. - Add regression tests: aggregate.test.ts covers matches spanning beyond the context window and multiple distinct matches; api.test.ts covers the fileContent handoff from fetchAllResults (success, failure, opt-out). See https://github.com/fulll/github-code-search/pull/154#discussion_r3838709033 See https://github.com/fulll/github-code-search/pull/154#discussion_r3838709043 See https://github.com/fulll/github-code-search/pull/154#discussion_r3838709048 See https://github.com/fulll/github-code-search/pull/154#discussion_r3838709051 --- github-code-search.ts | 10 ++++- src/aggregate.test.ts | 57 ++++++++++++++++++++++++++ src/aggregate.ts | 95 +++++++++++++++++++++++++++---------------- src/api.test.ts | 69 +++++++++++++++++++++++++++++++ src/api.ts | 9 +++- 5 files changed, 203 insertions(+), 37 deletions(-) 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 a125cd5..274d7bd 100644 --- a/src/aggregate.test.ts +++ b/src/aggregate.test.ts @@ -410,4 +410,61 @@ describe("aggregate — regexFilter fallback to full fileContent", () => { 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 6a17619..e75e419 100644 --- a/src/aggregate.ts +++ b/src/aggregate.ts @@ -80,40 +80,74 @@ function recomputeSegments( // ─── Aggregation ───────────────────────────────────────────────────────────── /** - * Extracts a small multi-line window of `content` around the 1-based line - * `matchLine`, mirroring the size of context GitHub's own fragment field - * typically provides. Used to build a display-friendly fallback `TextMatch` - * when the API-provided fragment does not contain a match but the full - * downloaded file content does — see issue #148. + * 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 sliceContextWindow( +function fallbackMatchesFromFullContent( content: string, - matchLine: number, + re: RegExp, contextLines = 2, -): { fragment: string; fragmentStartLine: number } { - const lines = content.split("\n"); - const startLine = Math.max(1, matchLine - contextLines); - const endLine = Math.min(lines.length, matchLine + contextLines); - return { - fragment: lines.slice(startLine - 1, endLine).join("\n"), - fragmentStartLine: startLine, +): 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"); -/** - * Finds the 1-based line numbers where `re` matches within `content`. - * `re` must be a global RegExp; its `lastIndex` is reset before use. - */ -function findMatchLines(content: string, re: RegExp): Set { - re.lastIndex = 0; - const lines = new Set(); + const results: TextMatch[] = []; let m: RegExpExecArray | null; while ((m = re.exec(content)) !== null) { - const before = content.slice(0, m.index); - lines.add((before.match(/\n/g)?.length ?? 0) + 1); - if (m[0].length === 0) re.lastIndex++; + 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 lines; + return results; } export function aggregate( @@ -166,14 +200,7 @@ export function aggregate( // 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) { - const fileContent = m.fileContent; - updatedTextMatches = [...findMatchLines(fileContent, globalRe)] - .map((matchLine) => { - const { fragment, fragmentStartLine } = sliceContextWindow(fileContent, matchLine); - const segs = recomputeSegments(fragment, globalRe, fragmentStartLine); - return segs.length > 0 ? { fragment, matches: segs } : null; - }) - .filter((tm): tm is TextMatch => tm !== null); + 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. 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 60ac874..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…")); @@ -280,8 +284,9 @@ export async function fetchAllResults( 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. - fileContent, + // 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;