diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index ce8945a5..519e2d67 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -82,6 +82,7 @@ - **perf index の一括追加をしない** — index を無計画に足すと ANALYZE 無しの planner heuristics が崩れて別 query が 30-50x 回帰した実績が複数ある。追加時は必ず対象 query と非対象 query の両方を実測すること - **`redirectPaths` の `slice(1)` を外さない** — follow-redirects の `res.redirects[0]` はクエリ落ちした元 URL であり、外すとクエリ違いの別ページが同一視され消失する(`crawler/src/crawler/fetch-destination.ts`。E2E: `/query-distinct/`) - **「ページか」は content-type で判定する。`isTarget` で判定しない** — `isTarget` は「in-scope なクロール対象か」であり、in-scope な PDF も `isTarget=1`(`crawler/src/archive/normalize-content-type.ts`、`query/src/list-pages.ts`) +- **`status = 404` はページ/コンテンツ集計にカウントしない** — 404 URL の背後にページは実在しないため、summary の総数・contentTypeDistribution・directory-tree(ノード・カウント・membership とも)から由来を問わず除外する。表示面は `statusDistribution` のみで、そこでは `source = 'inventory-seed'`(`--inventory` のインプットミス)を別行に分離する。充足率だけは非対称(seed 404 のみ分母から除外 — 修正対象 404 はメタデータ欠落として数え続ける)。カウントを持つ機能を今後追加する際もこの規則に従うこと(正: `query/src/get-summary.ts`、`viewer-read-model/build-directory-tree-rows.ts` の JSDoc) - **被リンクは redirect 透過解決するが、発リンクは解決しない** — 発リンク側の raw な指し先は「古い URL にリンクしている」という監査シグナル。この非対称性を「統一」しないこと(`database.ts`) - **被リンク系の集約粒度は referrer 単位で揃える** — `listInboundLinks`(`viewer_anchor_facts` は `(source_page_id, dest_page_id)` で一意)と `listExternalLinks.referrerCount`(`COUNT(DISTINCT source.id)`)は同一粒度。片方だけ anchor 単位に変えると外部リンク一覧の参照元数と被リンク件数が食い違う(`query/src/list-external-links.ts`、`list-inbound-links.ts`)。`getPageDetail` はこの集約自体を持たない(inbound links 全体を `listInboundLinks` に切り出し済み、issue #235) - **URL natural-sort comparator は推移律を保証しない** — 重複排除は `compareUrlSortKeys(...) === 0` ではなく `original` 文字列の完全一致で行い、`viewer_url_sort_keys` への INSERT は `onConflict('url').ignore()` を fail-safe に使う。viewer 起動時ソートは外部マージソート(メモリよりチャンクサイズ優先)で、結果は tar-cache 配下に JSONL ストリーミング永続化(`query/src/external-url-sort.ts`、`merge-sorted-url-chunks.ts`、`url-sort-temp-table.ts`、`viewer/src/url-sort-cache.ts`) diff --git a/packages/@nitpicker/mcp-server/src/tool-definitions.ts b/packages/@nitpicker/mcp-server/src/tool-definitions.ts index 9b8bff4d..8184957e 100644 --- a/packages/@nitpicker/mcp-server/src/tool-definitions.ts +++ b/packages/@nitpicker/mcp-server/src/tool-definitions.ts @@ -39,7 +39,7 @@ export const toolDefinitions: Tool[] = [ { name: 'get_summary', description: - 'Get site-wide overview. Returns: internal/external HTML page counts (`internalPages` / `externalPages`), internal/external content-row counts across every MIME (`internalContents` / `externalContents` — HTML + PDF + Office docs + CSVs + archives + ...), HTTP status distribution, Content-Type distribution over 18 categories (html, pdf, csv, word, excel, powerpoint, image, css, javascript, json+yaml, xml, font, audio, video, archive, text, other, unknown), and metadata fulfillment rates (title, description, OG tags) for internal HTML pages only. `internalContents` is always >= `internalPages` (the latter applies the historical HTML-or-null filter, the former does not). Use this first to understand the archive contents.', + 'Get site-wide overview. Returns: internal/external HTML page counts (`internalPages` / `externalPages`), internal/external content-row counts across every MIME (`internalContents` / `externalContents` — HTML + PDF + Office docs + CSVs + archives + ...), HTTP status distribution, Content-Type distribution over 18 categories (html, pdf, csv, word, excel, powerpoint, image, css, javascript, json+yaml, xml, font, audio, video, archive, text, other, unknown), and metadata fulfillment rates (title, description, OG tags) for internal HTML pages only. Every page/content count and the Content-Type distribution exclude `status = 404` rows (no page exists behind a 404 URL); 404s appear only in the status distribution, split into a plain `404` row (fix-target broken pages) and a trailing `inventorySeed: true` row (`crawl --inventory` input mistakes). The metadata denominator excludes only the inventory-seed 404s. `internalContents` is always >= `internalPages` (the latter applies the historical HTML-or-null filter, the former does not). Use this first to understand the archive contents.', inputSchema: { type: 'object' as const, properties: { diff --git a/packages/@nitpicker/query/src/get-summary.spec.ts b/packages/@nitpicker/query/src/get-summary.spec.ts index 3c880a71..ec96df5d 100644 --- a/packages/@nitpicker/query/src/get-summary.spec.ts +++ b/packages/@nitpicker/query/src/get-summary.spec.ts @@ -112,13 +112,18 @@ describe('getSummary', () => { isSkipped: false, }); + // 403, not 404: unlike a 404 (excluded from every total — see the + // dedicated "404 exclusion" describe below), a 403 page exists and + // must keep counting, which is exactly the legacy counting path this + // fixture pins: an errored-but-existing HTML page counts as a page, + // shows in the histogram, and stays in the metadata denominator. await archive.setPage({ - url: parseUrl('https://example.com/404')!, + url: parseUrl('https://example.com/403')!, redirectPaths: [], isExternal: false, isTarget: true, - status: 404, - statusText: 'Not Found', + status: 403, + statusText: 'Forbidden', contentType: 'text/html', contentLength: 100, responseHeaders: {}, @@ -328,7 +333,7 @@ describe('getSummary', () => { the same HTML-or-null filter, so adding the external PDF doesn't bump this. */ expect(result.statusDistribution).toContainEqual({ status: 200, count: 3 }); - expect(result.statusDistribution).toContainEqual({ status: 404, count: 1 }); + expect(result.statusDistribution).toContainEqual({ status: 403, count: 1 }); // Metadata denominator stays 3 (HTML pages only): neither the PDF nor the // errored page dilutes it (otherwise title would be 2/4 or 2/5). expect(result.metadataFulfillment.title).toBeCloseTo(2 / 3); @@ -373,6 +378,279 @@ describe('getSummary', () => { }); }); +describe('getSummary: 404 exclusion', () => { + let archive: InstanceType; + const dir = path.resolve(__dirname, '__test_fixtures_summary_404_exclusion__'); + const archiveFilePath = path.resolve(dir, 'summary-404-exclusion.nitpicker'); + + /** + * Insert an HTML page with the minimal meta shape `setPage` requires. + * @param params - The page's varying fields. + * @param params.url - The page URL. + * @param params.status - The HTTP status. + * @param params.isExternal - Whether the page is external. + * @param params.title - The page title, or `null` for none. + */ + async function insertHtmlPage(params: { + url: string; + status: number; + isExternal: boolean; + title: string | null; + }): Promise { + await archive.setPage({ + url: parseUrl(params.url)!, + redirectPaths: [], + isExternal: params.isExternal, + isTarget: !params.isExternal, + status: params.status, + statusText: params.status === 200 ? 'OK' : 'Not Found', + contentType: 'text/html', + contentLength: 100, + responseHeaders: {}, + html: `${params.title ?? ''}`, + meta: { + lang: null, + title: params.title, + description: null, + keywords: null, + noindex: false, + nofollow: false, + noarchive: false, + canonical: null, + alternate: null, + 'og:type': null, + 'og:title': null, + 'og:site_name': null, + 'og:description': null, + 'og:url': null, + 'og:image': null, + 'twitter:card': null, + }, + anchorList: [], + imageList: [], + isSkipped: false, + }); + } + + beforeAll(async () => { + const { mkdirSync } = await import('node:fs'); + mkdirSync(dir, { recursive: true }); + archive = await Archive.create({ filePath: archiveFilePath, cwd: dir }); + await archive.setConfig({ + baseUrl: 'https://example.com', + roots: ['https://example.com'], + name: 'test', + version: '0.13.0', + recursive: true, + interval: 0, + image: true, + fetchExternal: false, + parallels: 1, + excludes: [], + excludeKeywords: [], + excludeUrls: [], + maxExcludedDepth: 0, + retry: 3, + fromList: false, + disableQueries: false, + userAgent: 'test', + ignoreRobots: false, + }); + + await insertHtmlPage({ + url: 'https://example.com/', + status: 200, + isExternal: false, + title: 'Home', + }); + // Fix-target 404s (source stays `setPage`'s default `'crawled'`): + // one internal, one external — both must vanish from every total + // while merging into the single plain 404 histogram row. + await insertHtmlPage({ + url: 'https://example.com/gone', + status: 404, + isExternal: false, + title: null, + }); + await insertHtmlPage({ + url: 'https://other.example/dead', + status: 404, + isExternal: true, + title: null, + }); + // An inventory-seed 404 (an input mistake — the URL came from a + // `crawl --inventory` list and never existed). Relabelled directly + // (same precedent as the alias_of_id describe above): `setPage` + // always writes `'crawled'`, and going through the full + // `--inventory` orchestration here would drown the fixture in + // unrelated setup. + await insertHtmlPage({ + url: 'https://example.com/ghost', + status: 404, + isExternal: false, + title: null, + }); + const knex = archive.getKnex(); + await knex('content_items') + .whereIn('url_id', (qb) => { + qb.select('id').from('url_refs').where('url', 'https://example.com/ghost'); + }) + .update({ source: 'inventory-seed' }); + }); + + afterAll(async () => { + if (archive) { + await archive.close(); + } + const { rmSync } = await import('node:fs'); + rmSync(dir, { recursive: true, force: true }); + }); + + it('excludes every 404 from page/content totals regardless of provenance', async () => { + const result = await getSummary(archive); + // Only the 200 home page counts — /gone (crawled), /ghost + // (inventory-seed) and the external /dead are all 404s. + expect(result.totalPages).toBe(1); + expect(result.internalPages).toBe(1); + expect(result.externalPages).toBe(0); + expect(result.internalContents).toBe(1); + expect(result.externalContents).toBe(0); + expect(result.contentTypeDistribution).toEqual([ + { category: 'html', internal: 1, external: 0 }, + ]); + }); + + it('splits the 404 histogram row by provenance, trailing the inventory-seed row last', async () => { + const result = await getSummary(archive); + // The plain row merges the two fix-target 404s (internal + external); + // `toContainEqual` is exact-equality per element, so the two-field + // literal can only match the plain row — the seed row carries the + // extra `inventorySeed: true`. + expect(result.statusDistribution).toContainEqual({ status: 404, count: 2 }); + expect(result.statusDistribution.at(-1)).toEqual({ + status: 404, + count: 1, + inventorySeed: true, + }); + }); + + it('keeps fix-target 404s in the metadata denominator but drops inventory-seed 404s', async () => { + const result = await getSummary(archive); + // Denominator = home + /gone (a fix-target page still owes its + // metadata) = 2; /ghost is an input mistake and leaves. Only home + // has a title: 1/2. An all-404 exclusion would read 1/1 and a + // no-exclusion would read 1/3 — 1/2 pins the asymmetric rule. + expect(result.metadataFulfillment.title).toBeCloseTo(1 / 2); + }); +}); + +describe('getSummary: statusDistribution ordering with a null-status legacy row', () => { + let archive: InstanceType; + const dir = path.resolve(__dirname, '__test_fixtures_summary_null_status_order__'); + const archiveFilePath = path.resolve(dir, 'summary-null-status-order.nitpicker'); + + beforeAll(async () => { + const { mkdirSync } = await import('node:fs'); + mkdirSync(dir, { recursive: true }); + archive = await Archive.create({ filePath: archiveFilePath, cwd: dir }); + await archive.setConfig({ + baseUrl: 'https://example.com', + roots: ['https://example.com'], + name: 'test', + version: '0.13.0', + recursive: true, + interval: 0, + image: true, + fetchExternal: false, + parallels: 1, + excludes: [], + excludeKeywords: [], + excludeUrls: [], + maxExcludedDepth: 0, + retry: 3, + fromList: false, + disableQueries: false, + userAgent: 'test', + ignoreRobots: false, + }); + + for (const page of [ + { url: 'https://example.com/', status: 200 }, + { url: 'https://example.com/ghost', status: 404 }, + ]) { + await archive.setPage({ + url: parseUrl(page.url)!, + redirectPaths: [], + isExternal: false, + isTarget: true, + status: page.status, + statusText: page.status === 200 ? 'OK' : 'Not Found', + contentType: 'text/html', + contentLength: 100, + responseHeaders: {}, + html: '', + meta: { + lang: null, + title: null, + description: null, + keywords: null, + noindex: false, + nofollow: false, + noarchive: false, + canonical: null, + alternate: null, + 'og:type': null, + 'og:title': null, + 'og:site_name': null, + 'og:description': null, + 'og:url': null, + 'og:image': null, + 'twitter:card': null, + }, + anchorList: [], + imageList: [], + isSkipped: false, + }); + } + const knex = archive.getKnex(); + await knex('content_items') + .whereIn('url_id', (qb) => { + qb.select('id').from('url_refs').where('url', 'https://example.com/ghost'); + }) + .update({ source: 'inventory-seed' }); + // A raw legacy-shaped row with NO status (and no content type): + // `setPage` cannot produce one, so insert directly — the same + // technique build-viewer-read-model.spec uses for its unparseable-URL + // row. + const [urlRef] = await knex('url_refs') + .insert({ url: 'https://example.com/legacy-null' }) + .returning('id'); + await knex('content_items').insert({ + url_id: urlRef.id, + scraped: 1, + is_target: 1, + is_external: 0, + }); + }); + + afterAll(async () => { + if (archive) { + await archive.close(); + } + const { rmSync } = await import('node:fs'); + rmSync(dir, { recursive: true, force: true }); + }); + + it('orders rows: numeric statuses, then the inventory-seed 404, then the null trailer', async () => { + const result = await getSummary(archive); + expect(result.statusDistribution).toEqual([ + { status: 200, count: 1 }, + { status: 404, count: 1, inventorySeed: true }, + { status: null, count: 1 }, + ]); + }); +}); + describe('getSummary: HTMLページが1つも無い(全てエラー/到達不能)アーカイブ', () => { let archive: InstanceType; const dir = path.resolve(__dirname, '__test_fixtures_summary_no_html__'); diff --git a/packages/@nitpicker/query/src/get-summary.ts b/packages/@nitpicker/query/src/get-summary.ts index f0828652..6cb63005 100644 --- a/packages/@nitpicker/query/src/get-summary.ts +++ b/packages/@nitpicker/query/src/get-summary.ts @@ -6,7 +6,7 @@ import type { StatusCount, SummaryResult, } from './types.js'; -import type { ArchiveAccessor, ErrorKind } from '@nitpicker/crawler'; +import type { ArchiveAccessor, ErrorKind, PageSource } from '@nitpicker/crawler'; import { classifyErrorKind, isWithinOutageWindow } from '@nitpicker/crawler'; @@ -32,6 +32,16 @@ import { resolveFailedPageMessages } from './resolve-failed-page-messages.js'; * URL-normalization is no more "its own page" for counting purposes than an * HTTP redirect source is. * + * `status = 404` rows are excluded from every page/content total and from + * `contentTypeDistribution` regardless of provenance — no page exists behind + * a 404 URL. They remain visible only in `statusDistribution`, split into + * two rows: the plain `404` row (fix-target broken pages) and a trailing + * {@link StatusCount.inventorySeed} row (`source = 'inventory-seed'` — URLs + * that came from a `crawl --inventory` list and never existed, i.e. input + * mistakes). `metadataFulfillment` applies a narrower, deliberately + * asymmetric rule: only the inventory-seed 404s leave the denominator — + * fix-target 404s still owe their metadata (see {@link SummaryResult}). + * * Each `errorKindBreakdown` entry (on the `status === -1` row) carries a * {@link FailureAttribution}: `'site'` unless the failure's message * timestamp falls inside a recorded `network_outages` window, in which @@ -85,7 +95,11 @@ export async function getSummary(accessor: ArchiveAccessor): Promise { qb.whereNull('ctr.raw').orWhere('ctr.raw', 'text/html'); }) - .groupBy('ci.is_external', 'ci.status') as Promise< - { isExternal: 0 | 1; status: number | null; count: number | string }[] + // `source` in the grouping key only serves the 404 seed/non-seed + // split below — for every other status the JS accumulator merges + // the per-source rows right back together. + .groupBy('ci.is_external', 'ci.status', 'ci.source') as Promise< + { + isExternal: 0 | 1; + status: number | null; + // PageSource (not string) so a typo in the JS-side + // 'inventory-seed' comparison below is a TS no-overlap error + // instead of a silently-never-matching branch. + source: PageSource; + count: number | string; + }[] >, knex('content_items as ci') .join('content_type_refs as ctr', 'ctr.id', 'ci.content_type_id') .leftJoin('page_meta as pm', 'pm.page_id', 'ci.id') + // Inventory-seed 404s (input mistakes — no such page should + // exist) must not dilute the fulfillment rates, but every OTHER + // 404 stays: a fix-target broken page still owes its metadata. + // NULL-status legacy rows fall through the first branch and stay + // counted. (`source` is NOT NULL DEFAULT 'crawled', so it needs + // no NULL branch of its own.) + .where((qb) => { + qb.whereNull('ci.status') + .orWhereNot('ci.status', 404) + .orWhereNot('ci.source', 'inventory-seed'); + }) .select( knex.raw('COUNT(*) as total'), knex.raw( @@ -132,6 +168,12 @@ export async function getSummary(accessor: ArchiveAccessor): Promise excludeSkippedPages(qb, 'ci.is_skipped')) + // No page exists behind a 404 URL, whatever its provenance, so + // content totals skip them entirely. NULL-status legacy rows are + // not 404s and stay counted. + .where((qb) => { + qb.whereNull('ci.status').orWhereNot('ci.status', 404); + }) .groupBy('ctr.raw', 'ci.is_external') as Promise< { contentType: string | null; @@ -148,9 +190,22 @@ export async function getSummary(accessor: ArchiveAccessor): Promise(); for (const row of pageRows) { const n = Number(row.count); + // 404s never count as pages (no page exists behind a 404 URL) but + // they DO stay in the status histogram — split by provenance so + // inventory-list input mistakes don't masquerade as fix-target + // broken pages. + if (row.status === 404) { + if (row.source === 'inventory-seed') { + inventorySeed404Num += n; + } else { + statusAcc.set(404, (statusAcc.get(404) ?? 0) + n); + } + continue; + } totalNum += n; if (row.isExternal === 1) { externalNum += n; @@ -159,17 +214,37 @@ export async function getSummary(accessor: ArchiveAccessor): Promise ({ status, count })) - .toSorted((a, b) => { - if (a.status === null) { - return 1; - } - if (b.status === null) { - return -1; - } - return a.status - b.status; - }); + /** + * Sort tier of one status row: `0` for regular numeric statuses, + * `1` for the inventory-seed 404 trailer, `2` for the `null`-status + * trailer — "real statuses first, input noise last, unknown last of + * all" (see {@link StatusCount.inventorySeed}). NOT plain + * `a.status - b.status`: that would slot the seed row between 403 and + * 405 instead of after every regular row. + * @param entry - The row to rank. + * @returns The tier. + */ + function statusSortTier(entry: StatusCount): number { + if (entry.status === null) { + return 2; + } + return entry.inventorySeed ? 1 : 0; + } + const statusDistribution: StatusCount[] = [ + ...[...statusAcc.entries()].map(([status, count]) => ({ status, count })), + ...(inventorySeed404Num > 0 + ? [{ status: 404, count: inventorySeed404Num, inventorySeed: true as const }] + : []), + ].toSorted((a, b) => { + const tierDelta = statusSortTier(a) - statusSortTier(b); + if (tierDelta !== 0) { + return tierDelta; + } + // Within tier 0, ascending by status; tiers 1 and 2 hold at most + // one row each (single seed bucket, single null bucket), so the + // 0-fallback never reorders anything real. + return (a.status ?? 0) - (b.status ?? 0); + }); const minusOneEntry = statusDistribution.find((e) => e.status === -1); let networkOutageAffectedFailures = 0; diff --git a/packages/@nitpicker/query/src/get-viewer-summary.spec.ts b/packages/@nitpicker/query/src/get-viewer-summary.spec.ts index d1b4ff02..5e58d32b 100644 --- a/packages/@nitpicker/query/src/get-viewer-summary.spec.ts +++ b/packages/@nitpicker/query/src/get-viewer-summary.spec.ts @@ -111,13 +111,16 @@ describe('getViewerSummary', () => { isSkipped: false, }); + // 403, not 404: unlike a 404 (excluded from every total — see + // getSummary), a 403 page exists and must keep counting, which is + // exactly the legacy counting path this fixture pins. await archive.setPage({ url: parseUrl('https://example.net/')!, redirectPaths: [], isExternal: true, isTarget: false, - status: 404, - statusText: 'Not Found', + status: 403, + statusText: 'Forbidden', contentType: 'text/html', contentLength: 0, responseHeaders: {}, @@ -152,7 +155,7 @@ describe('getViewerSummary', () => { // two implementations agree, but would not catch a bug shared by // both. These hardcoded literals pin the actual expected values // independently, derived by hand from the fixture above (home: - // 200/internal/html/title+description, example.net: 404/external/html). + // 200/internal/html/title+description, example.net: 403/external/html). const result = await getViewerSummary(archive); expect(result).toMatchObject({ totalPages: 2, @@ -162,7 +165,7 @@ describe('getViewerSummary', () => { externalContents: 1, statusDistribution: [ { status: 200, count: 1 }, - { status: 404, count: 1 }, + { status: 403, count: 1 }, ], contentTypeDistribution: [{ category: 'html', internal: 1, external: 1 }], metadataFulfillment: { @@ -286,4 +289,78 @@ describe('getViewerSummary', () => { }); }); }); + + describe('with an inventory-seed 404 in the archive', () => { + const workingDir = path.resolve( + __dirname, + '__test_fixtures_get_viewer_summary_seed_404__', + ); + const archiveFilePath = path.resolve(workingDir, 'seed-404-test.nitpicker'); + let archive: InstanceType; + + beforeAll(async () => { + const { mkdirSync } = await import('node:fs'); + mkdirSync(workingDir, { recursive: true }); + archive = await Archive.create({ filePath: archiveFilePath, cwd: workingDir }); + await archive.setConfig(BASE_CONFIG); + + for (const page of [ + { url: 'https://example.com/', status: 200 }, + { url: 'https://example.com/gone', status: 404 }, + { url: 'https://example.com/ghost', status: 404 }, + ]) { + await archive.setPage({ + url: parseUrl(page.url)!, + redirectPaths: [], + isExternal: false, + isTarget: true, + status: page.status, + statusText: page.status === 200 ? 'OK' : 'Not Found', + contentType: 'text/html', + contentLength: 100, + responseHeaders: {}, + html: '', + meta: META, + anchorList: [], + imageList: [], + isSkipped: false, + }); + } + // Relabel /ghost's provenance directly — `setPage` always writes + // `'crawled'`, and the full `--inventory` orchestration would + // drown this fixture in unrelated setup. + await archive + .getKnex()('content_items') + .whereIn('url_id', (qb) => { + qb.select('id').from('url_refs').where('url', 'https://example.com/ghost'); + }) + .update({ source: 'inventory-seed' }); + + await buildViewerReadModel(archive); + }); + + afterAll(async () => { + if (archive) { + await archive.releaseHandle(); + } + const { rmSync } = await import('node:fs'); + rmSync(workingDir, { recursive: true, force: true }); + }); + + it('round-trips the inventorySeed split row through viewer_summary.status_json intact', async () => { + const [viewerSummary, liveSummary] = await Promise.all([ + getViewerSummary(archive), + getSummary(archive), + ]); + expect(viewerSummary).toEqual(liveSummary); + // Hardcoded literals so a serialisation bug shared by both + // implementations cannot hide: the crawled /gone stays in the + // plain 404 row, the seed /ghost splits into the trailer. + expect(viewerSummary.statusDistribution).toEqual([ + { status: 200, count: 1 }, + { status: 404, count: 1 }, + { status: 404, count: 1, inventorySeed: true }, + ]); + }); + }); }); diff --git a/packages/@nitpicker/query/src/list-directory-pages.ts b/packages/@nitpicker/query/src/list-directory-pages.ts index 21ce99d3..ae5415da 100644 --- a/packages/@nitpicker/query/src/list-directory-pages.ts +++ b/packages/@nitpicker/query/src/list-directory-pages.ts @@ -78,6 +78,10 @@ async function joinDirectoryPageIdsToListItems( * `page_id` tie-breaker) — forward-only, unlike `listViewerPages`'s * bidirectional keyset, since this endpoint has no virtual-scroll-upward * requirement. + * + * `status = 404` pages never appear here: they are dropped at read-model + * build time (see `buildDirectoryTreeRows`), not filtered per request — use + * the Pages view's status filter to locate 404s. * @param accessor - The archive accessor to query. * @param options - See {@link ListDirectoryPagesOptions}. * @returns Up to `limit` pages plus a `nextCursor` for continuation, or diff --git a/packages/@nitpicker/query/src/types.ts b/packages/@nitpicker/query/src/types.ts index a7017c6a..ca7b4001 100644 --- a/packages/@nitpicker/query/src/types.ts +++ b/packages/@nitpicker/query/src/types.ts @@ -556,7 +556,10 @@ export interface SummaryResult { /** * Total number of HTML pages (internal + external) — the historical * "pages" count, restricted to `contentType IS NULL OR text/html` so - * PDFs / images / archives don't inflate it. Kept on the API for + * PDFs / images / archives don't inflate it, and excluding every + * `status = 404` row regardless of provenance (a 404 URL has no page + * behind it; the 404s themselves stay visible in + * {@link SummaryResult.statusDistribution}). Kept on the API for * backward compatibility (CLI / MCP consumers may surface it * directly); the viewer dashboard now prefers * {@link SummaryResult.internalPages} / {@link SummaryResult.internalContents} / @@ -565,40 +568,56 @@ export interface SummaryResult { totalPages: number; /** * Number of internal HTML pages (`isExternal = 0` AND - * `contentType IS NULL OR text/html`). This is the "real pages we - * crawled and rendered" number, excluding non-HTML targets like - * PDFs or downloads. + * `contentType IS NULL OR text/html` AND `status != 404`). This is the + * "real pages we crawled and rendered" number, excluding non-HTML + * targets like PDFs or downloads and excluding 404s (no page exists + * behind a 404 URL). */ internalPages: number; /** - * Number of external HTML pages (`isExternal = 1` AND HTML-or-null). - * Kept for backward compatibility; the viewer now prefers - * {@link SummaryResult.externalContents} which counts every external link - * regardless of MIME. + * Number of external HTML pages (`isExternal = 1` AND HTML-or-null AND + * `status != 404`). Kept for backward compatibility; the viewer now + * prefers {@link SummaryResult.externalContents} which counts every + * external link regardless of MIME. */ externalPages: number; /** * Number of internal **content rows** (every `isExternal = 0` page * in the archive — HTML pages plus typed non-HTML targets such as - * PDFs, CSVs, ZIPs, Office docs). This is the broader "how much - * stuff lives under the in-scope domains" number, with no MIME - * filter. Always `>= internalPages`. + * PDFs, CSVs, ZIPs, Office docs), excluding `status = 404` rows. This + * is the broader "how much stuff lives under the in-scope domains" + * number, with no MIME filter. Always `>= internalPages`. */ internalContents: number; /** * Number of external **content rows** (every `isExternal = 1` page - * in the archive, any MIME). This is the "how many distinct - * outbound links did we find" number. Always `>= externalPages`. + * in the archive, any MIME, excluding `status = 404` rows). This is + * the "how many distinct outbound links did we find" number. Always + * `>= externalPages`. */ externalContents: number; - /** Distribution of HTTP status codes across all pages. */ + /** + * Distribution of HTTP status codes across all pages. 404s are counted + * here (and only here — every total above excludes them), split into two + * rows by provenance: the plain `status === 404` row counts fix-target + * 404s, and a trailing {@link StatusCount.inventorySeed} row counts + * `source = 'inventory-seed'` input mistakes. + */ statusDistribution: StatusCount[]; - /** Metadata fulfillment rates for internal pages. */ + /** + * Metadata fulfillment rates for internal pages. The denominator + * excludes `status = 404 AND source = 'inventory-seed'` rows (input + * mistakes — no such page should exist, so its missing metadata is not + * a quality signal) but KEEPS every other 404 (a fix-target broken page + * still owes its metadata) — deliberately asymmetric with the count + * fields above, which exclude all 404s as "not real pages". + */ metadataFulfillment: MetadataFulfillment; /** * Distribution of {@link ContentTypeCategory} across all in-scope rows - * (HTML pages plus known non-HTML targets such as PDFs). Sorted by total - * count descending so the dominant types lead the chart. + * (HTML pages plus known non-HTML targets such as PDFs), excluding + * `status = 404` rows the same way the count fields above do. Sorted by + * total count descending so the dominant types lead the chart. */ contentTypeDistribution: ContentTypeCount[]; /** @@ -635,6 +654,17 @@ export interface StatusCount { status: number | null; /** Number of pages with this status code. */ count: number; + /** + * Present (and always `true`) only on the extra `status === 404` row that + * counts `source = 'inventory-seed'` rows — URLs that came straight from a + * `crawl --inventory` list and turned out not to exist, i.e. input + * mistakes rather than pages the site ever linked to. The plain + * `status === 404` row counts every OTHER 404 (fix-target broken pages). + * The two rows never merge; this one is ordered after every regular + * status row (but before the `status === null` row, when present) so the + * histogram reads "real statuses first, input noise last". + */ + inventorySeed?: true; /** * Per-cause breakdown of the `status === -1` bucket, classified by * {@link import('@nitpicker/crawler').classifyErrorKind} on the underlying message. @@ -3071,6 +3101,12 @@ export interface BuildViewerReadModelOptions { * and {@link listDirectoryChildren}. `parentNodeId` is the only structural * link — callers reconstruct the nested UI tree client-side from this flat * list, since neither endpoint recurses server-side. + * + * Every count column excludes `status = 404` rows — no page exists behind a + * 404 URL, so it is neither counted nor attached as a membership, and a + * directory whose pages are all 404s has no node at all (the build-time + * rule lives in `buildDirectoryTreeRows`; 404s remain reachable through the + * Pages view's status filter). */ export interface DirectoryTreeNode { /** This node's unique id — stable across `getDirectoryTree`/`listDirectoryChildren` calls. */ diff --git a/packages/@nitpicker/query/src/viewer-read-model/build-directory-tree-rows.spec.ts b/packages/@nitpicker/query/src/viewer-read-model/build-directory-tree-rows.spec.ts index 2e3e1a8e..d70fa470 100644 --- a/packages/@nitpicker/query/src/viewer-read-model/build-directory-tree-rows.spec.ts +++ b/packages/@nitpicker/query/src/viewer-read-model/build-directory-tree-rows.spec.ts @@ -6,22 +6,33 @@ import { buildDirectoryTreeRows } from './build-directory-tree-rows.js'; /** * Shorthand for a {@link DirectoryTreeSourceRow} fixture row — `isExternal` - * defaults to `0` (internal) and `contentType` defaults to `'text/html'` + * defaults to `0` (internal), `contentType` defaults to `'text/html'` * (so existing fixtures count toward `*_html_page_count` unless a test - * explicitly overrides it) when omitted. - * @param id - The row's `pages.id`. - * @param url - The row's URL. - * @param isExternal - Optional `isExternal` override. - * @param contentType - Optional raw MIME override. + * explicitly overrides it), and `status` defaults to `200` when omitted. + * Defaults apply only when a key is absent — an explicit `null` (the + * legacy-row shape several tests pin) is passed through as-is. + * @param params - The row's fields; only `id` and `url` are required. + * @param params.id - The row's `pages.id`. + * @param params.url - The row's URL. + * @param params.isExternal - Optional `isExternal` override. + * @param params.contentType - Optional raw MIME override. + * @param params.status - Optional HTTP status override. * @returns The fixture row. */ -function row( - id: number, - url: string, - isExternal: number | null = 0, - contentType: string | null = 'text/html', -): DirectoryTreeSourceRow { - return { id, url, isExternal, contentType }; +function row(params: { + id: number; + url: string; + isExternal?: number | null; + contentType?: string | null; + status?: number | null; +}): DirectoryTreeSourceRow { + return { + id: params.id, + url: params.url, + isExternal: params.isExternal === undefined ? 0 : params.isExternal, + contentType: params.contentType === undefined ? 'text/html' : params.contentType, + status: params.status === undefined ? 200 : params.status, + }; } describe('buildDirectoryTreeRows', () => { @@ -30,7 +41,9 @@ describe('buildDirectoryTreeRows', () => { }); it('creates exactly one depth-0 root node for a single root-only page', () => { - const { nodes, pages } = buildDirectoryTreeRows([row(1, 'https://example.com/')]); + const { nodes, pages } = buildDirectoryTreeRows([ + row({ id: 1, url: 'https://example.com/' }), + ]); expect(nodes).toHaveLength(1); expect(nodes[0]).toMatchObject({ parent_node_id: null, @@ -58,8 +71,8 @@ describe('buildDirectoryTreeRows', () => { it('lands a no-trailing-slash page and a trailing-slash page on the same directory node', () => { const { nodes, pages } = buildDirectoryTreeRows([ - row(1, 'https://example.com/blog/2024/post-1'), - row(2, 'https://example.com/blog/2024/'), + row({ id: 1, url: 'https://example.com/blog/2024/post-1' }), + row({ id: 2, url: 'https://example.com/blog/2024/' }), ]); const leaf = nodes.find((n) => n.path === '/blog/2024/'); expect(leaf).toMatchObject({ depth: 2, direct_page_count: 2 }); @@ -68,7 +81,7 @@ describe('buildDirectoryTreeRows', () => { it('creates intermediate directories with zero direct pages of their own', () => { const { nodes } = buildDirectoryTreeRows([ - row(1, 'https://example.com/a/b/c/d/page'), + row({ id: 1, url: 'https://example.com/a/b/c/d/page' }), ]); const byPath = new Map(nodes.map((n) => [n.path, n])); expect(byPath.get('/')).toMatchObject({ @@ -100,9 +113,9 @@ describe('buildDirectoryTreeRows', () => { it('propagates descendant counts bottom-up through every ancestor', () => { const { nodes } = buildDirectoryTreeRows([ - row(1, 'https://example.com/blog/2024/post-1'), - row(2, 'https://example.com/blog/2024/post-2'), - row(3, 'https://example.com/blog/'), + row({ id: 1, url: 'https://example.com/blog/2024/post-1' }), + row({ id: 2, url: 'https://example.com/blog/2024/post-2' }), + row({ id: 3, url: 'https://example.com/blog/' }), ]); const byPath = new Map(nodes.map((n) => [n.path, n])); expect(byPath.get('/blog/2024/')).toMatchObject({ @@ -121,8 +134,8 @@ describe('buildDirectoryTreeRows', () => { it('splits descendant counts into internal/external, summing to descendant_page_count', () => { const { nodes } = buildDirectoryTreeRows([ - row(1, 'https://example.com/'), - row(2, 'https://example.com/legacy/old.html', 1), + row({ id: 1, url: 'https://example.com/' }), + row({ id: 2, url: 'https://example.com/legacy/old.html', isExternal: 1 }), ]); const root = nodes.find((n) => n.path === '/')!; expect(root).toMatchObject({ @@ -140,8 +153,8 @@ describe('buildDirectoryTreeRows', () => { it('includes a same-host, out-of-scope (external) page in its host tree once the host qualifies', () => { const { nodes, pages } = buildDirectoryTreeRows([ - row(1, 'https://example.com/'), - row(2, 'https://example.com/legacy/old.html', 1), + row({ id: 1, url: 'https://example.com/' }), + row({ id: 2, url: 'https://example.com/legacy/old.html', isExternal: 1 }), ]); expect(nodes.some((n) => n.root_key === 'example.com' && n.path === '/legacy/')).toBe( true, @@ -151,30 +164,36 @@ describe('buildDirectoryTreeRows', () => { it('excludes a host with zero internal pages entirely — no nodes, no pages', () => { const { nodes, pages } = buildDirectoryTreeRows([ - row(1, 'https://twitter.com/someaccount', 1), + row({ id: 1, url: 'https://twitter.com/someaccount', isExternal: 1 }), ]); expect(nodes).toEqual([]); expect(pages).toEqual([]); }); it('treats a null isExternal as internal (legacy pre-backfill rows)', () => { - const { nodes } = buildDirectoryTreeRows([row(1, 'https://example.com/', null)]); + const { nodes } = buildDirectoryTreeRows([ + row({ id: 1, url: 'https://example.com/', isExternal: null }), + ]); expect(nodes.some((n) => n.root_key === 'example.com')).toBe(true); const root = nodes.find((n) => n.path === '/')!; expect(root.internal_descendant_page_count).toBe(1); }); it('skips a row with an unparseable URL without throwing', () => { - expect(() => buildDirectoryTreeRows([row(1, 'not a valid url')])).not.toThrow(); - const { nodes, pages } = buildDirectoryTreeRows([row(1, 'not a valid url')]); + expect(() => + buildDirectoryTreeRows([row({ id: 1, url: 'not a valid url' })]), + ).not.toThrow(); + const { nodes, pages } = buildDirectoryTreeRows([ + row({ id: 1, url: 'not a valid url' }), + ]); expect(nodes).toEqual([]); expect(pages).toEqual([]); }); it('builds two independent, non-colliding trees for two qualifying hosts', () => { const { nodes } = buildDirectoryTreeRows([ - row(1, 'https://example.com/'), - row(2, 'https://example.org/'), + row({ id: 1, url: 'https://example.com/' }), + row({ id: 2, url: 'https://example.org/' }), ]); expect(nodes).toHaveLength(2); const nodeIds = nodes.map((n) => n.node_id); @@ -186,8 +205,8 @@ describe('buildDirectoryTreeRows', () => { it('merges two different subpaths of the same host into one tree as siblings (multi-root crawl)', () => { const { nodes } = buildDirectoryTreeRows([ - row(1, 'https://example.com/blog/index'), - row(2, 'https://example.com/news/index'), + row({ id: 1, url: 'https://example.com/blog/index' }), + row({ id: 2, url: 'https://example.com/news/index' }), ]); const roots = nodes.filter((n) => n.parent_node_id === null); expect(roots).toHaveLength(1); @@ -199,7 +218,9 @@ describe('buildDirectoryTreeRows', () => { }); it('sets has_children to 0 for a leaf directory that has direct pages but no child directories', () => { - const { nodes } = buildDirectoryTreeRows([row(1, 'https://example.com/a/b')]); + const { nodes } = buildDirectoryTreeRows([ + row({ id: 1, url: 'https://example.com/a/b' }), + ]); // No trailing slash: 'b' is a page filename, so '/a/' is the leaf // directory here — it has 1 direct page and 0 child directories. const a = nodes.find((n) => n.path === '/a/')!; @@ -211,7 +232,9 @@ describe('buildDirectoryTreeRows', () => { }); it('sets has_children to 1 for a directory that has a child directory, even with zero direct pages of its own', () => { - const { nodes } = buildDirectoryTreeRows([row(1, 'https://example.com/a/b/c')]); + const { nodes } = buildDirectoryTreeRows([ + row({ id: 1, url: 'https://example.com/a/b/c' }), + ]); const a = nodes.find((n) => n.path === '/a/')!; expect(a).toMatchObject({ direct_child_dir_count: 1, @@ -222,9 +245,17 @@ describe('buildDirectoryTreeRows', () => { it('counts only html-classified rows toward direct_html_page_count, unlike direct_page_count', () => { const { nodes } = buildDirectoryTreeRows([ - row(1, 'https://example.com/docs/page.html', 0, 'text/html'), - row(2, 'https://example.com/docs/photo.jpg', 0, 'image/jpeg'), - row(3, 'https://example.com/docs/doc.pdf', 0, 'application/pdf'), + row({ id: 1, url: 'https://example.com/docs/page.html' }), + row({ + id: 2, + url: 'https://example.com/docs/photo.jpg', + contentType: 'image/jpeg', + }), + row({ + id: 3, + url: 'https://example.com/docs/doc.pdf', + contentType: 'application/pdf', + }), ]); const docs = nodes.find((n) => n.path === '/docs/')!; expect(docs).toMatchObject({ @@ -237,8 +268,12 @@ describe('buildDirectoryTreeRows', () => { it('propagates descendant_html_page_count bottom-up, excluding non-html descendants that still count toward descendant_page_count', () => { const { nodes } = buildDirectoryTreeRows([ - row(1, 'https://example.com/blog/2024/post.html', 0, 'text/html'), - row(2, 'https://example.com/blog/2024/banner.jpg', 0, 'image/jpeg'), + row({ id: 1, url: 'https://example.com/blog/2024/post.html' }), + row({ + id: 2, + url: 'https://example.com/blog/2024/banner.jpg', + contentType: 'image/jpeg', + }), ]); const byPath = new Map(nodes.map((n) => [n.path, n])); expect(byPath.get('/blog/2024/')).toMatchObject({ @@ -257,9 +292,51 @@ describe('buildDirectoryTreeRows', () => { }); }); + it('excludes a 404 row entirely — no counts, no page membership', () => { + const { nodes, pages } = buildDirectoryTreeRows([ + row({ id: 1, url: 'https://example.com/' }), + row({ id: 2, url: 'https://example.com/gone', status: 404 }), + ]); + const root = nodes.find((n) => n.path === '/')!; + expect(root).toMatchObject({ + direct_page_count: 1, + descendant_page_count: 1, + internal_descendant_page_count: 1, + direct_html_page_count: 1, + descendant_html_page_count: 1, + }); + expect(pages.some((p) => p.page_id === 2)).toBe(false); + }); + + it('creates no node for a directory whose pages are all 404s', () => { + const { nodes } = buildDirectoryTreeRows([ + row({ id: 1, url: 'https://example.com/' }), + row({ id: 2, url: 'https://example.com/removed/old-1', status: 404 }), + row({ id: 3, url: 'https://example.com/removed/old-2', status: 404 }), + ]); + expect(nodes.some((n) => n.path === '/removed/')).toBe(false); + }); + + it('does not let an internal 404 qualify its host — a host with only 404 internal pages has no tree', () => { + const { nodes, pages } = buildDirectoryTreeRows([ + row({ id: 1, url: 'https://example.com/gone', status: 404 }), + row({ id: 2, url: 'https://example.com/linked-from-elsewhere', isExternal: 1 }), + ]); + expect(nodes).toEqual([]); + expect(pages).toEqual([]); + }); + + it('keeps a NULL-status legacy row — only a literal 404 is excluded', () => { + const { nodes, pages } = buildDirectoryTreeRows([ + row({ id: 1, url: 'https://example.com/', status: null }), + ]); + expect(nodes.find((n) => n.path === '/')).toMatchObject({ direct_page_count: 1 }); + expect(pages).toHaveLength(1); + }); + it('ignores query strings and hashes when resolving the directory chain', () => { const { nodes } = buildDirectoryTreeRows([ - row(1, 'https://example.com/blog/2024/post-1?utm_source=x#section'), + row({ id: 1, url: 'https://example.com/blog/2024/post-1?utm_source=x#section' }), ]); expect(nodes.some((n) => n.path === '/blog/2024/')).toBe(true); expect(nodes.find((n) => n.path === '/blog/2024/')).toMatchObject({ diff --git a/packages/@nitpicker/query/src/viewer-read-model/build-directory-tree-rows.ts b/packages/@nitpicker/query/src/viewer-read-model/build-directory-tree-rows.ts index fc2b12b7..12a70ca6 100644 --- a/packages/@nitpicker/query/src/viewer-read-model/build-directory-tree-rows.ts +++ b/packages/@nitpicker/query/src/viewer-read-model/build-directory-tree-rows.ts @@ -259,11 +259,18 @@ function propagateDescendantCounts(nodes: readonly DirectoryNodeInsertRow[]): vo * request time — all derivation cost is paid once at build time, keeping * reads to plain indexed SELECTs. * - * A host is included in the output ONLY if at least one of its rows has - * `isExternal` falsy (an "internal" page) — hosts that exist purely as - * external link targets (e.g. a social-media profile linked from the site) - * are excluded entirely, since a directory tree of a domain the crawl never - * actually visited has no value. Once a host qualifies, BOTH its internal + * `status = 404` rows are dropped before anything else — no counts, no + * `viewer_directory_pages` membership, no host qualification. No page exists + * behind a 404 URL, so a directory whose pages are all 404s gets no node and + * a host whose internal rows are all 404s gets no tree (the fix-target 404s + * remain reachable through the Pages view's status filter, just not through + * this feature). + * + * A host is included in the output ONLY if at least one of its (non-404) + * rows has `isExternal` falsy (an "internal" page) — hosts that exist purely + * as external link targets (e.g. a social-media profile linked from the + * site) are excluded entirely, since a directory tree of a domain the crawl + * never actually visited has no value. Once a host qualifies, BOTH its internal * and external rows are included in that host's tree: crawl scope is a * `(hostname, port, path)` triple (see `@nitpicker/crawler`'s * `find-scope-entry.ts`), so a same-host, out-of-scope subpath is @@ -292,6 +299,12 @@ export function buildDirectoryTreeRows( ): DirectoryTreeBuildResult { const parsedRows: ParsedPageRow[] = []; for (const row of rows) { + // A 404 URL has no page behind it, whatever its provenance — drop it + // before host eligibility so a host whose only internal rows are + // 404s builds no tree at all. NULL-status legacy rows are not 404s. + if (row.status === 404) { + continue; + } const parsed = parsePageRow(row); if (parsed) { parsedRows.push(parsed); diff --git a/packages/@nitpicker/query/src/viewer-read-model/build-viewer-read-model.spec.ts b/packages/@nitpicker/query/src/viewer-read-model/build-viewer-read-model.spec.ts index dcb71b65..2c2ff386 100644 --- a/packages/@nitpicker/query/src/viewer-read-model/build-viewer-read-model.spec.ts +++ b/packages/@nitpicker/query/src/viewer-read-model/build-viewer-read-model.spec.ts @@ -1056,6 +1056,28 @@ describe('buildViewerReadModel', () => { }); } + // A 404 page in a directory of its own — must be excluded from the + // tree end-to-end (SELECT → source-row mapping → builder): no + // /removed/ node, no membership row. Every count assertion in this + // describe doubles as the regression net: if the exclusion broke, + // this page would create a node and shift the totals below. + await archive.setPage({ + url: parseUrl('https://example.com/removed/gone')!, + redirectPaths: [], + isExternal: false, + isTarget: true, + status: 404, + statusText: 'Not Found', + contentType: 'text/html', + contentLength: 0, + responseHeaders: {}, + html: '', + meta: META, + anchorList: [], + imageList: [], + isSkipped: false, + }); + await buildViewerReadModel(archive); }); @@ -1190,10 +1212,30 @@ describe('buildViewerReadModel', () => { count: '*', }); // 7 attached pages: root, blog x1, blog/2024 x2, legacy, news, a/b/c/d — - // the unparseable-URL row and the twitter.com row contribute none. + // the unparseable-URL row, the twitter.com row and the 404 + // /removed/gone row contribute none. expect(Number(total[0]?.count)).toBe(7); }); + it('excludes a 404 page from the tree end-to-end — no node for its directory, no membership row', async () => { + const knex = archive.getKnex(); + // The page still exists as an ordinary viewer_pages row (the Pages + // view keeps listing 404s) — only the directory tree drops it. + const pages = await knex('viewer_pages') + .where('url', 'https://example.com/removed/gone') + .select('page_id'); + expect(pages).toHaveLength(1); + + expect( + await knex('viewer_directory_nodes').where('path', '/removed/').select('*'), + ).toEqual([]); + expect( + await knex('viewer_directory_pages') + .where('page_id', pages[0].page_id) + .select('*'), + ).toEqual([]); + }); + it('rebuilds the directory tree idempotently — a second build leaves the same node/page counts and root counts', async () => { const knex = archive.getKnex(); const nodesBefore = await knex('viewer_directory_nodes').count<{ count: string }[]>( @@ -1570,13 +1612,16 @@ describe('buildViewerReadModel', () => { isSkipped: false, }); + // 403, not 404: unlike a 404 (excluded from every total — see + // getSummary), a 403 page exists and must keep counting, which is + // exactly the legacy counting path this fixture pins. await archive.setPage({ url: parseUrl('https://example.net/')!, redirectPaths: [], isExternal: true, isTarget: false, - status: 404, - statusText: 'Not Found', + status: 403, + statusText: 'Forbidden', contentType: 'text/html', contentLength: 0, responseHeaders: {}, @@ -1649,7 +1694,7 @@ describe('buildViewerReadModel', () => { // proves the two implementations agree, but would not catch a bug // shared by both. These hardcoded literals — derived by hand from // the 3-page fixture above (home: 200/internal/html/full-metadata, - // example.net: 404/external/html, broken: -1/internal/null-contentType) + // example.net: 403/external/html, broken: -1/internal/null-contentType) // — pin the actual expected values independently. await buildViewerReadModel(archive); const row = await archive.getKnex()('viewer_summary').where('id', 1).first(); @@ -1670,7 +1715,7 @@ describe('buildViewerReadModel', () => { ).toEqual([ { status: -1, count: 1 }, { status: 200, count: 1 }, - { status: 404, count: 1 }, + { status: 403, count: 1 }, ]); const contentTypeDistribution: { diff --git a/packages/@nitpicker/query/src/viewer-read-model/build-viewer-read-model.ts b/packages/@nitpicker/query/src/viewer-read-model/build-viewer-read-model.ts index 73f731ff..ec83e4c6 100644 --- a/packages/@nitpicker/query/src/viewer-read-model/build-viewer-read-model.ts +++ b/packages/@nitpicker/query/src/viewer-read-model/build-viewer-read-model.ts @@ -577,6 +577,7 @@ export async function buildViewerReadModel( url: row.url, isExternal: row.isExternal, contentType: row.contentType, + status: row.status, })), ); for (let start = 0; start < directoryNodes.length; start += INSERT_CHUNK_SIZE) { diff --git a/packages/@nitpicker/query/src/viewer-read-model/types.ts b/packages/@nitpicker/query/src/viewer-read-model/types.ts index b872c8c0..ef3070d6 100644 --- a/packages/@nitpicker/query/src/viewer-read-model/types.ts +++ b/packages/@nitpicker/query/src/viewer-read-model/types.ts @@ -88,6 +88,14 @@ export interface DirectoryTreeSourceRow { * without conflating them with crawled images/PDFs/etc. */ contentType: string | null; + /** + * `content_items.status`. Rows with a literal `404` are excluded from + * the tree entirely — no counts, no `viewer_directory_pages` membership, + * no host qualification — because no page exists behind a 404 URL (the + * same rule `getSummary` applies to its totals). `null` (legacy rows + * predating the column) is NOT a 404 and stays included. + */ + status: number | null; } /** diff --git a/packages/@nitpicker/query/src/viewer-read-model/viewer-read-model-schema-version.ts b/packages/@nitpicker/query/src/viewer-read-model/viewer-read-model-schema-version.ts index 801ebbbc..3a0c7e18 100644 --- a/packages/@nitpicker/query/src/viewer-read-model/viewer-read-model-schema-version.ts +++ b/packages/@nitpicker/query/src/viewer-read-model/viewer-read-model-schema-version.ts @@ -7,4 +7,4 @@ * `viewer_read_model_meta.schema_version` to decide whether a rebuild is * needed. */ -export const VIEWER_READ_MODEL_SCHEMA_VERSION = 23; +export const VIEWER_READ_MODEL_SCHEMA_VERSION = 24; diff --git a/packages/@nitpicker/viewer/web/components/build-status-row-descriptor.spec.ts b/packages/@nitpicker/viewer/web/components/build-status-row-descriptor.spec.ts new file mode 100644 index 00000000..7183d42b --- /dev/null +++ b/packages/@nitpicker/viewer/web/components/build-status-row-descriptor.spec.ts @@ -0,0 +1,42 @@ +import { describe, expect, it } from 'vitest'; + +import { buildStatusRowDescriptor } from './build-status-row-descriptor.js'; + +describe('buildStatusRowDescriptor', () => { + it('uses the numeric status as both key and label for a regular row', () => { + expect(buildStatusRowDescriptor({ status: 200, count: 10 })).toEqual({ + key: '200', + label: '200', + }); + }); + + it('keeps the plain 404 row (fix-target broken pages) undecorated', () => { + expect(buildStatusRowDescriptor({ status: 404, count: 2 })).toEqual({ + key: '404', + label: '404', + }); + }); + + it('suffixes the inventory-seed 404 row so its key cannot collide with the plain 404 row', () => { + expect( + buildStatusRowDescriptor({ status: 404, count: 1200, inventorySeed: true }), + ).toEqual({ + key: '404-inventory-seed', + label: '404 (inventory-seed)', + }); + }); + + it('maps a null status to the "none" key and an em-dash label', () => { + expect(buildStatusRowDescriptor({ status: null, count: 1 })).toEqual({ + key: 'none', + label: '—', + }); + }); + + it('renders the -1 hard-failure sentinel like any other numeric status', () => { + expect(buildStatusRowDescriptor({ status: -1, count: 3 })).toEqual({ + key: '-1', + label: '-1', + }); + }); +}); diff --git a/packages/@nitpicker/viewer/web/components/build-status-row-descriptor.ts b/packages/@nitpicker/viewer/web/components/build-status-row-descriptor.ts new file mode 100644 index 00000000..9fdb1219 --- /dev/null +++ b/packages/@nitpicker/viewer/web/components/build-status-row-descriptor.ts @@ -0,0 +1,30 @@ +import type { StatusCount } from '@nitpicker/query'; + +/** + * Derives the React `key` and display label for one status-distribution row + * on the Summary view. Kept out of the component so the two derivations are + * unit-testable and cannot drift: the same `inventorySeed` check drives + * both, because the seed row shares `status: 404` with the fix-target row — + * keying on `status` alone would collide in React's reconciliation, and + * labelling on `status` alone would render two indistinguishable "404" + * rows. + * @param entry - The status-distribution entry to present. + * @returns The unique `key` and human-readable `label` for the row. + * @example + * buildStatusRowDescriptor({ status: 404, count: 2 }); + * // => { key: '404', label: '404' } + * buildStatusRowDescriptor({ status: 404, count: 1200, inventorySeed: true }); + * // => { key: '404-inventory-seed', label: '404 (inventory-seed)' } + * buildStatusRowDescriptor({ status: null, count: 1 }); + * // => { key: 'none', label: '—' } + */ +export function buildStatusRowDescriptor(entry: StatusCount): { + key: string; + label: string; +} { + const base = entry.status === null ? null : String(entry.status); + if (entry.inventorySeed) { + return { key: `${base}-inventory-seed`, label: `${base} (inventory-seed)` }; + } + return { key: base ?? 'none', label: base ?? '—' }; +} diff --git a/packages/@nitpicker/viewer/web/routes/summary-view.tsx b/packages/@nitpicker/viewer/web/routes/summary-view.tsx index 41122440..7da07591 100644 --- a/packages/@nitpicker/viewer/web/routes/summary-view.tsx +++ b/packages/@nitpicker/viewer/web/routes/summary-view.tsx @@ -1,6 +1,7 @@ import type { MetadataFulfillment } from '@nitpicker/query'; import { useSummary } from '../api/use-summary.js'; +import { buildStatusRowDescriptor } from '../components/build-status-row-descriptor.js'; import { ContentTypeStackedBar } from '../components/content-type-stacked-bar.js'; import { ViewHeader } from '../components/view-header.js'; import { getAttributionLabel } from '../i18n/get-attribution-label.js'; @@ -72,6 +73,12 @@ const METADATA_LABELS: { key: keyof MetadataFulfillment; label: string }[] = [ * glance, the same way the macOS / iOS storage view does. All percent * labels go through {@link formatPercent} so precision and the * sub-0.1%-but-non-zero edge case read consistently across groups. + * + * The status group can contain two 404 rows: the plain `404` row + * (fix-target broken pages) and a trailing `404 (inventory-seed)` row + * (input mistakes from a `crawl --inventory` list — see + * `StatusCount.inventorySeed`). The card totals above never include 404s + * of either kind, so the histogram is the only place they surface here. * @returns The summary view element. */ export function SummaryView() { @@ -156,13 +163,14 @@ export function SummaryView() {
{data.statusDistribution.map((entry) => { const ratio = computeRatio(entry.count, statusTotal); + const { key, label } = buildStatusRowDescriptor(entry); const showBreakdown = entry.status === -1 && entry.errorKindBreakdown !== undefined && entry.errorKindBreakdown.length > 0; return (
- {entry.status ?? '—'} + {/* Fixed width 60 for every row keeps the bar tracks + aligned; the long inventory-seed label wraps inside + it instead of pushing its bar out of column. */} + {label}