diff --git a/components/ads/slot-manager.tsx b/components/ads/slot-manager.tsx index c959611..0a60604 100644 --- a/components/ads/slot-manager.tsx +++ b/components/ads/slot-manager.tsx @@ -115,6 +115,16 @@ export function SlotManager({ const [repoChoices, setRepoChoices] = useState< { owner: string; repo: string; installation_id: number }[] | null >(null); + // Set when the installer can't find a document shell on its own. The + // publisher knows where theirs is, so we ask rather than dead-ending, and + // remember which repo the failed attempt was against so the retry matches. + const [askPath, setAskPath] = useState(false); + const [targetPath, setTargetPath] = useState(""); + const [lastRepo, setLastRepo] = useState<{ + owner: string; + repo: string; + installation_id: number; + } | null>(null); const snippets = slot ? snippetsFor(fmt, slot.id, origin) : []; // The first recipe is the canonical one for each unit, so an unset selection @@ -187,15 +197,19 @@ export function SlotManager({ } } - async function submitPr(pick?: { owner: string; repo: string; installation_id: number }) { + async function submitPr( + pick?: { owner: string; repo: string; installation_id: number }, + path?: string, + ) { if (!slot) return; + const repo = pick ?? lastRepo ?? undefined; setPrBusy(true); setPrMsg(null); try { const res = await fetch(`/api/ads/slots/${slot.id}/install-embed`, { method: "POST", headers: { "Content-Type": "application/json" }, - body: JSON.stringify(pick ?? {}), + body: JSON.stringify({ ...(repo ?? {}), ...(path ? { target_path: path } : {}) }), }); const json = await res.json(); if (!res.ok) { @@ -207,8 +221,12 @@ export function SlotManager({ return; } setRepoChoices(null); + if (repo) setLastRepo(repo); const r = json.data; - setPrMsg({ ok: true, text: r.detail, url: r.prUrl }); + // Discovery came up empty (or the path given was wrong): ask for the + // file instead of leaving the publisher with a message and no button. + setAskPath(Boolean(r.needsTargetPath)); + setPrMsg({ ok: !r.needsTargetPath, text: r.detail, url: r.prUrl }); } catch (e) { setPrMsg({ ok: false, text: e instanceof Error ? e.message : "Network error." }); } finally { @@ -343,6 +361,38 @@ export function SlotManager({ + {askPath && ( +
+
+ Which file closes your </body>? Repo-relative, e.g.{" "} + src/app.html or apps/web/app/layout.tsx. +
+
{ + e.preventDefault(); + const path = targetPath.trim(); + if (path) submitPr(undefined, path); + }} + > + setTargetPath(e.target.value)} + placeholder="app/layout.tsx" + aria-label="Path to the file that closes the document" + /> + +
+
+ )} + {repoChoices && (
Choose a repo:
diff --git a/lib/github/install-ad.ts b/lib/github/install-ad.ts index 671088d..d32b6c4 100644 --- a/lib/github/install-ad.ts +++ b/lib/github/install-ad.ts @@ -81,6 +81,8 @@ export interface InstallAdResult { path?: string; /** CSP config files patched so the ad unit isn't blocked. */ cspPaths?: string[]; + /** Discovery found nothing: the UI should ask for a file path and retry. */ + needsTargetPath?: boolean; detail: string; } @@ -163,7 +165,17 @@ export async function installAdEmbed(input: InstallAdInput): Promise tag was found in the repo." }; + // Discovery probes the canonical layout paths, every template-shaped file + // in the repo tree, and code search. When all three come up empty the + // publisher knows where their shell is and we don't, so say so and let + // them name it (the caller passes it back as targetPath). + return { + status: "noop", + needsTargetPath: true, + detail: + `No file with a tag was found in ${input.owner}/${input.repo} on ${base}. ` + + "If your document shell lives somewhere unusual, give the file path and we'll install there.", + }; } const file = await getFileContent({ @@ -174,7 +186,12 @@ export async function installAdEmbed(input: InstallAdInput): Promise tag in ${file.path}.` }; + return { + status: "noop", + path: file.path, + needsTargetPath: true, + detail: `No tag in ${file.path}, so there's nowhere to put the units. Try another file.`, + }; } // Patch the site's CSP so the browser can load /ad.js, reach /api/ads/serve, diff --git a/lib/github/install-tracker.ts b/lib/github/install-tracker.ts index 8485088..cba67bd 100644 --- a/lib/github/install-tracker.ts +++ b/lib/github/install-tracker.ts @@ -9,6 +9,7 @@ import { createBranch, getFileContent, getRepo, + listRepoTree, openPullRequest, putFile, searchRepoCode, @@ -74,6 +75,37 @@ const CANDIDATES: string[] = [ "src/views/Layout.tsx", "src/views/layout.jsx", "app/views/Layout.jsx", + // Next.js layouts are not always TypeScript. + "app/layout.js", + "src/app/layout.js", + "pages/_document.js", + "src/pages/_document.js", + // Remix / React Router: the document shell is the root route. + "app/root.tsx", + "app/root.jsx", + // SvelteKit renders every page into one HTML shell. + "src/app.html", + // Nuxt, when a project overrides the generated document. + "app.html", + // Astro, beyond the two names already probed above. + "src/layouts/Base.astro", + "src/layouts/MainLayout.astro", + "src/layouts/main.astro", + // Hugo without the _default indirection. + "layouts/baseof.html", + // Eleventy. + "_includes/base.njk", + "_includes/layout.njk", + "src/_includes/base.njk", + "src/_includes/layout.njk", + // Rails. + "app/views/layouts/application.html.erb", + // Django / Flask / anything Jinja. + "templates/base.html", + // Laravel Blade. + "resources/views/layouts/app.blade.php", + // A WordPress theme closes the document in footer.php. + "footer.php", "index.html", "public/index.html", "src/index.html", @@ -279,9 +311,15 @@ function cspCandidatePaths(root: string): string[] { function rankCandidatePath(path: string, repoName: string): number { let score = 0; // Strong negatives — almost never the live site's template. - if (/(^|\/)(boilerplates?|examples?|templates?|samples?|fixtures?|__tests__|tests?|spec|stories|playground|sandbox|demo|node_modules)(\/|$)/i.test(path)) { + // `templates/` is deliberately absent: for Django, Flask, Jinja and Rails + // that directory is exactly where the document shell lives, and penalizing + // it hid the only installable file in those repos. + if (/(^|\/)(boilerplates?|examples?|samples?|fixtures?|__tests__|tests?|spec|stories|playground|sandbox|demo|node_modules)(\/|$)/i.test(path)) { score -= 100; } + // Build output and vendored copies are never the site's own template, and a + // tree scan surfaces plenty of both. + if (SKIP_DIR_RE.test(path)) score -= 100; // Penalize markdown / docs paths just in case. if (/(^|\/)(docs?|documentation)(\/|$)/i.test(path)) score -= 20; // Likely a real app dir. @@ -300,11 +338,96 @@ function rankCandidatePath(path: string, repoName: string): number { // A views/Layout shell is as canonical for a Hono app as app/layout is for // Next, and it sits deeper, so it needs the same nudge not to lose on length. if (/\/views\/layout\.(tsx|jsx)$/i.test(path)) score += 10; + // The same nudge for the other frameworks' one true shell, each of which + // sits deep enough to lose to noise on path length alone. + if (/(^|\/)src\/app\.html$/i.test(path)) score += 15; // SvelteKit + if (/(^|\/)app\/root\.(tsx|jsx)$/i.test(path)) score += 10; // Remix + if (/(^|\/)baseof\.html$/i.test(path)) score += 10; // Hugo + if (/(^|\/)footer\.php$/i.test(path)) score += 5; // WordPress theme + // Whatever the framework, a file under a templates dir is a better guess + // than a component that merely happens to be named the same. + if (TEMPLATE_DIR_RE.test(path)) score += 5; // Shorter paths slightly preferred (closer to root = more canonical). score -= path.length * 0.05; return score; } +// Scanning the tree means fetching files to see whether they close a document, +// so the filter below decides what is worth a request. + +// Extensions that are essentially always a page or document template. +const DOCUMENT_EXT_RE = + /\.(x?html?|astro|ejs|hbs|handlebars|liquid|njk|nunjucks|twig|erb|mustache|eta|gohtml|tmpl|cshtml|razor|edge)$/i; + +// Extensions that are usually a component rather than a document. One of these +// only earns a request when its name or its directory says "document shell". +const COMPONENT_EXT_RE = /\.([cm]?[jt]sx?|vue|svelte|php)$/i; + +// The names a document shell goes by, across frameworks. Matched against the +// first dot-segment, so application.html.erb and _document.tsx both work. +const SHELL_NAME_RE = + /^(_?document|_?app|layout|root|base|baseof|default|shell|template|footer|head|html|entry-server)$/i; + +// Directories that hold templates whatever the stack. +const TEMPLATE_DIR_RE = + /(^|\/)(layouts?|views?|templates?|_layouts|_includes|partials|themes?)(\/|$)/i; + +// Never worth a request: build output, vendored code, test fixtures. +const SKIP_DIR_RE = + /(^|\/)(node_modules|\.git|\.next|\.nuxt|\.svelte-kit|\.astro|\.cache|\.vercel|dist|build|out|coverage|vendor|third_party|storybook-static|\.storybook|__snapshots__|__fixtures__|__mocks__)(\/|$)/i; + +/** How many tree-discovered files we will open looking for a . */ +const MAX_TREE_PROBES = 30; + +/** Parallel GETs against the contents API. Polite, and well under the limit. */ +const PROBE_CONCURRENCY = 6; + +/** + * Template-shaped paths from a repo tree, best-first and capped. + * + * The canonical list only knows the conventions someone thought to write + * down. A repo that keeps its shell somewhere else — or uses a framework + * nobody here has met — still has a file that closes the document, and the + * tree names every path in the repo for one request. + */ +function templateShapedPaths( + files: string[], + root: string, + repoName: string, +): string[] { + const prefix = root ? `${root}/` : ""; + return files + .filter((p) => (prefix ? p.startsWith(prefix) : true)) + .filter((p) => !SKIP_DIR_RE.test(p)) + .filter((p) => { + const name = p.slice(p.lastIndexOf("/") + 1); + if (DOCUMENT_EXT_RE.test(name)) return true; + if (!COMPONENT_EXT_RE.test(name)) return false; + return SHELL_NAME_RE.test(name.split(".")[0]) || TEMPLATE_DIR_RE.test(p); + }) + .sort((a, b) => rankCandidatePath(b, repoName) - rankCandidatePath(a, repoName)) + .slice(0, MAX_TREE_PROBES); +} + +/** Run an async map over items, at most `limit` in flight. */ +async function mapWithConcurrency( + items: T[], + limit: number, + fn: (item: T) => Promise, +): Promise { + const out = new Array(items.length); + let next = 0; + const worker = async () => { + for (let i = next++; i < items.length; i = next++) { + out[i] = await fn(items[i]); + } + }; + await Promise.all( + Array.from({ length: Math.min(limit, items.length) }, worker), + ); + return out; +} + export interface InstallCandidate { path: string; /** Score from rankCandidatePath — debug / explainer. */ @@ -333,17 +456,53 @@ export async function findInstallCandidates(input: { const root = normalizeRoot(input.rootPath); const canonical = candidatePaths(root); - const found = new Map(); - - // Probe the canonical list — cheap, no rate limit on contents API. - for (const path of canonical) { - const file = await getFileContent({ + // One request lists every path in the repo, which answers two questions the + // canonical probes can't: which of those paths actually exist (so we skip + // the misses), and where a repo that follows none of our conventions keeps + // its shell. Unlike code search this works on private repos, which is most + // of them. + let tree: Awaited> = null; + try { + tree = await listRepoTree({ token: input.token, owner: input.owner, repo: input.repo, - path, ref, }); + } catch { + // The tree is an optimization on top of a fallback; losing it only costs + // us the probes we were making before. + } + + let probes: string[]; + if (tree) { + const present = new Set(tree.files); + probes = [ + // A truncated tree is only a prefix of the repo, so absence from it + // proves nothing — keep probing the whole canonical list. + ...(tree.truncated ? canonical : canonical.filter((p) => present.has(p))), + ...templateShapedPaths(tree.files, root, input.repo), + ]; + } else { + probes = canonical; + } + + const found = new Map(); + + // Open each candidate and keep the ones that actually close a document. + const files = await mapWithConcurrency( + [...new Set(probes)], + PROBE_CONCURRENCY, + (path) => + getFileContent({ + token: input.token, + owner: input.owner, + repo: input.repo, + path, + ref, + }), + ); + for (const file of files) { if (file && /<\/body>/i.test(file.content)) { found.set(file.path, { sizeBytes: file.content.length }); } @@ -691,11 +850,44 @@ export async function installTracker(input: InstallInput): Promise + // 2. Fallback: read the repo tree and probe anything template-shaped. The + // canonical list above is a list of conventions, and a repo is free to + // keep its shell somewhere none of them predict. This runs before code + // search because it works on private repos, which search does not index. + if (!target && !alreadyInstalledPath) { + let tree: Awaited> = null; + try { + tree = await listRepoTree({ + token: input.token, + owner: input.owner, + repo: input.repo, + ref: base, + }); + } catch { + // Non-fatal: code search below is still there to try. + } + if (tree) { + const seen = new Set(candidates); + for (const path of templateShapedPaths(tree.files, root, input.repo)) { + if (seen.has(path)) continue; + const r = await probe(path); + if (r.kind === "already") { + alreadyInstalledPath = r.path; + break; + } + if (r.kind === "hit") { + target = r.file; + break; + } + } + } + } + + // 3. Fallback: ask GitHub's code search for any file containing // in this repo. Handles monorepos (e.g. apps/web/app/layout.tsx, // sites/foo/app/layout.tsx) and non-standard frameworks // (SvelteKit src/app.html, Remix app/root.tsx, ...). - if (!target) { + if (!target && !alreadyInstalledPath) { try { const hits = await searchRepoCode({ token: input.token, @@ -729,12 +921,12 @@ export async function installTracker(input: InstallInput): Promise found in ${input.owner}/${input.repo}. Probed canonical paths: ${probed}. ${hint}`, + `No template file with found in ${input.owner}/${input.repo} on ${base}. ` + + `Searched the canonical layout paths, every template-shaped file in the repo, and code search. ${hint}`, ); } diff --git a/tests/contract/install-ad.test.ts b/tests/contract/install-ad.test.ts index af6ee00..5a43a5c 100644 --- a/tests/contract/install-ad.test.ts +++ b/tests/contract/install-ad.test.ts @@ -16,6 +16,12 @@ const github = vi.hoisted(() => { return { path, sha: `sha-${path}`, content }; }), searchRepoCode: vi.fn(async () => []), + // The tree lists exactly the files this fake repo has, which is what the + // real one does and what discovery now leans on. + listRepoTree: vi.fn(async () => ({ + files: [...files.keys()], + truncated: false, + })), createBranch: vi.fn(async () => ({ created: true })), putFile: vi.fn(async ({ path }: { path: string; contentUtf8: string }) => ({ content: { sha: `new-sha-${path}`, path }, @@ -33,6 +39,7 @@ vi.mock("@/lib/github/repos", () => ({ getRepo: github.getRepo, getFileContent: github.getFileContent, searchRepoCode: github.searchRepoCode, + listRepoTree: github.listRepoTree, createBranch: github.createBranch, putFile: github.putFile, openPullRequest: github.openPullRequest, @@ -75,6 +82,7 @@ describe("installAdEmbed", () => { github.getRepo.mockClear(); github.getFileContent.mockClear(); github.searchRepoCode.mockClear(); + github.listRepoTree.mockClear(); github.createBranch.mockClear(); github.putFile.mockClear(); github.openPullRequest.mockClear(); @@ -156,6 +164,56 @@ describe("installAdEmbed", () => { expect(content.lastIndexOf("data-cp-ad")).toBeLessThan(content.indexOf("")); }); + it("installs into a shell the canonical paths do not know about", async () => { + github.files.set( + "site/templates/shell.html", + "\n\n
\n\n", + ); + + const result = await installAdEmbed({ + token: "token", + owner: "owner", + repo: "repo", + slotId: "slot-abc", + }); + + expect(result.status).toBe("opened"); + expect(result.path).toBe("site/templates/shell.html"); + }); + + it("asks for a path instead of dead-ending when nothing closes a document", async () => { + github.files.set("README.md", "# no html here\n"); + + const result = await installAdEmbed({ + token: "token", + owner: "owner", + repo: "repo", + slotId: "slot-abc", + }); + + expect(result.status).toBe("noop"); + expect(result.needsTargetPath).toBe(true); + expect(github.openPullRequest).not.toHaveBeenCalled(); + }); + + it("installs at an explicitly named path", async () => { + github.files.set( + "weird/place/Doc.tsx", + "export const Doc = () => ;\n", + ); + + const result = await installAdEmbed({ + token: "token", + owner: "owner", + repo: "repo", + slotId: "slot-abc", + targetPath: "weird/place/Doc.tsx", + }); + + expect(result.status).toBe("opened"); + expect(result.path).toBe("weird/place/Doc.tsx"); + }); + it("no-ops when the embed exists and no CSP needs changes", async () => { github.files.set( "app/layout.tsx", diff --git a/tests/contract/install-tracker.test.ts b/tests/contract/install-tracker.test.ts index 140a70f..1b9a3ae 100644 --- a/tests/contract/install-tracker.test.ts +++ b/tests/contract/install-tracker.test.ts @@ -16,6 +16,12 @@ const github = vi.hoisted(() => { return { path, sha: `sha-${path}`, content }; }), searchRepoCode: vi.fn(async () => []), + // The tree lists exactly the files this fake repo has, which is what the + // real one does and what discovery now leans on. + listRepoTree: vi.fn(async () => ({ + files: [...files.keys()], + truncated: false, + })), createBranch: vi.fn(async () => ({ created: true })), putFile: vi.fn(async ({ path }: { path: string; contentUtf8: string }) => ({ content: { sha: `new-sha-${path}`, path }, @@ -33,6 +39,7 @@ vi.mock("@/lib/github/repos", () => ({ getRepo: github.getRepo, getFileContent: github.getFileContent, searchRepoCode: github.searchRepoCode, + listRepoTree: github.listRepoTree, createBranch: github.createBranch, putFile: github.putFile, openPullRequest: github.openPullRequest, @@ -50,6 +57,7 @@ describe("install tracker candidate discovery", () => { github.getRepo.mockClear(); github.getFileContent.mockClear(); github.searchRepoCode.mockClear(); + github.listRepoTree.mockClear(); github.createBranch.mockClear(); github.putFile.mockClear(); github.openPullRequest.mockClear(); @@ -90,6 +98,76 @@ describe("install tracker candidate discovery", () => { ); }); + it("finds a shell no convention predicts, by scanning the repo tree", async () => { + // Nothing in the canonical list points here, and code search does not + // index private repos — the tree is the only thing that can find it. + github.files.set( + "server/render/document.tsx", + "export const Document = ({ children }) => {children};\n", + ); + + const candidates = await findInstallCandidates({ + token: "token", + owner: "owner", + repo: "repo", + }); + + expect(candidates.map((c) => c.path)).toContain("server/render/document.tsx"); + expect(github.searchRepoCode).toHaveBeenCalledOnce(); + }); + + it("finds a Django-style templates/base.html instead of penalizing it", async () => { + github.files.set( + "templates/base.html", + "\n{% block content %}{% endblock %}\n", + ); + + const candidates = await findInstallCandidates({ + token: "token", + owner: "owner", + repo: "repo", + }); + + expect(candidates.map((c) => c.path)).toContain("templates/base.html"); + }); + + it("never offers build output or vendored copies", async () => { + github.files.set( + "app/layout.tsx", + "export default function RootLayout({ children }) {\n return {children};\n}\n", + ); + github.files.set("dist/index.html", "built\n"); + github.files.set( + "node_modules/some-pkg/index.html", + "vendored\n", + ); + + const candidates = await findInstallCandidates({ + token: "token", + owner: "owner", + repo: "repo", + }); + + const paths = candidates.map((c) => c.path); + expect(paths[0]).toBe("app/layout.tsx"); + expect(paths).not.toContain("dist/index.html"); + expect(paths).not.toContain("node_modules/some-pkg/index.html"); + }); + + it("only opens files that exist when the tree says so", async () => { + github.files.set( + "app/layout.tsx", + "export default function RootLayout({ children }) {\n return {children};\n}\n", + ); + + await findInstallCandidates({ token: "token", owner: "owner", repo: "repo" }); + + // One request for the one real file, rather than a miss for every + // convention we have ever written down. + const probed = github.getFileContent.mock.calls.map((c) => c[0].path); + expect(probed).toEqual(["app/layout.tsx"]); + }); + it("does not double-prefix common monorepo candidates when rootPath is set", async () => { github.files.set( "apps/web/src/app/layout.tsx",