From c29b75e2d689bc322bf0190752194e31944511d6 Mon Sep 17 00:00:00 2001 From: Fayner Brack Date: Mon, 6 Jul 2026 00:24:37 +1000 Subject: [PATCH] fix(save-pipeline): count user-facing save failures + stop generate-summary DLQ loop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Emit view_save_intent outcome="error" on every synchronous save-validation 422 (queue save bar, extension save-article/save-html/save-content) so the save funnel finally has an error count; buildSaveIntentEvent now derives article_host via a null-safe parser, making article_host/content_class nullable (dashboards already coalesce these). Adds a "Save errors by surface" dashboard widget. In generate-summary, when canonical content is absent AND the aggregate row's crawl.kind is failed/unsupported, mark the summary skipped (crawl-failed/ crawl-unsupported) instead of asserting — this terminal collapse stops the auto-heal from repriming the row forever and ends the ~170/mo DLQ loop. The crawl=pending race and crawl=ready inconsistency keep the assert -> DLQ path. Claude-Session: https://claude.ai/code/session_01En2uUiiwQoRQKWenCnkPi9 --- .../observability/analytics-dashboard.test.ts | 4 +- .../observability/analytics-dashboard.ts | 14 ++ .../src/runtime/web/pages/queue/queue.page.ts | 9 + .../queue/queue.save-intent.route.test.ts | 156 ++++++++++++++++++ .../generate-summary-handler.test.ts | 61 +++++++ .../generate-summary-handler.ts | 24 +++ .../web-analytics/src/analytics.test.ts | 5 + src/packages/web-analytics/src/analytics.ts | 10 +- .../web-analytics/src/content-source.test.ts | 25 ++- .../web-analytics/src/content-source.ts | 16 ++ 10 files changed, 316 insertions(+), 8 deletions(-) diff --git a/projects/hutch/src/runtime/observability/analytics-dashboard.test.ts b/projects/hutch/src/runtime/observability/analytics-dashboard.test.ts index 1d1da71f3..2cea0fb14 100644 --- a/projects/hutch/src/runtime/observability/analytics-dashboard.test.ts +++ b/projects/hutch/src/runtime/observability/analytics-dashboard.test.ts @@ -70,9 +70,9 @@ function collectReferencedEvents(): Set { } describe("buildAnalyticsDashboardBody — drift prevention", () => { - it("emits 28 widgets (7 traffic+audience, 3 conversions, 3 imports+medium, 3 subscriptions, 2 view-funnel, 1 internal-clicks, 3 save-funnel, 1 summary-engagement, 2 audience-device, 1 errors, 1 homepage-ab, 1 blog-traffic) — adding or dropping one without updating this count is a deliberate signal to review the dashboard's scope", () => { + it("emits 29 widgets (7 traffic+audience, 3 conversions, 3 imports+medium, 3 subscriptions, 2 view-funnel, 1 internal-clicks, 4 save-funnel, 1 summary-engagement, 2 audience-device, 1 errors, 1 homepage-ab, 1 blog-traffic) — adding or dropping one without updating this count is a deliberate signal to review the dashboard's scope", () => { const body = buildBody(); - expect(body.widgets).toHaveLength(28); + expect(body.widgets).toHaveLength(29); }); it("the homepage A/B widget counts pageviews on the experiment campaign, grouped by variant (utm_content)", () => { diff --git a/projects/hutch/src/runtime/observability/analytics-dashboard.ts b/projects/hutch/src/runtime/observability/analytics-dashboard.ts index 812decf69..b1a3ec0cb 100644 --- a/projects/hutch/src/runtime/observability/analytics-dashboard.ts +++ b/projects/hutch/src/runtime/observability/analytics-dashboard.ts @@ -586,6 +586,20 @@ export function buildAnalyticsDashboardBody(deps: BuildAnalyticsDashboardDeps): x: 0, y: 122, width: 12, height: 8, view: "bar", }), + logWidget({ + region, + title: "Save errors by surface (view_save_intent outcome=error)", + logGroupNames: [hutchLogGroupName], + query: [ + `fields coalesce(surface, "${SAVE_SURFACES.readerView}") as surface`, + `| filter stream = "${STREAMS.analytics}" and event = "${ANALYTICS_EVENTS.viewSaveIntent}" and outcome = "${SAVE_OUTCOMES.error}"`, + ...exclude, + "| stats count(*) as save_errors by surface", + "| sort save_errors desc", + ].join(" "), + x: 0, y: 130, width: 12, height: 8, + view: "bar", + }), ); // --- Blog traffic --- diff --git a/projects/hutch/src/runtime/web/pages/queue/queue.page.ts b/projects/hutch/src/runtime/web/pages/queue/queue.page.ts index 3b95c2092..a29f02826 100644 --- a/projects/hutch/src/runtime/web/pages/queue/queue.page.ts +++ b/projects/hutch/src/runtime/web/pages/queue/queue.page.ts @@ -660,6 +660,7 @@ export function initQueueRoutes(deps: QueueDependencies): Router { const validation = deps.validateSaveableUrl(submittedUrl); if (validation.status === "ERROR") { + emitSaveIntent({ req, url: submittedUrl, path: SAVE_INTENT_PATH.saveArticle, surface: SAVE_SURFACES.extension, outcome: SAVE_OUTCOMES.error }); if (validation.error.code === "malformed_url") { res.status(422).type(SIREN_MEDIA_TYPE).json( sirenError({ code: "invalid-url", message: validation.error.message }), @@ -904,6 +905,7 @@ export function initQueueRoutes(deps: QueueDependencies): Router { res.status(201).type(SIREN_MEDIA_TYPE).json(toArticleEntity(result.saved)); return; } + emitSaveIntent({ req, url: typeof req.body?.url === "string" ? req.body.url : "", path: SAVE_INTENT_PATH.saveHtml, surface: SAVE_SURFACES.extension, outcome: SAVE_OUTCOMES.error }); res.status(422).type(SIREN_MEDIA_TYPE).json( sirenError({ code: "invalid-save-html", message: "Invalid save-html request" }), ); @@ -912,6 +914,7 @@ export function initQueueRoutes(deps: QueueDependencies): Router { const urlValidation = deps.validateSaveableUrl(parsed.data.url); if (urlValidation.status === "ERROR") { + emitSaveIntent({ req, url: parsed.data.url, path: SAVE_INTENT_PATH.saveHtml, surface: SAVE_SURFACES.extension, outcome: SAVE_OUTCOMES.error }); res.status(422).type(SIREN_MEDIA_TYPE).json( sirenError({ code: "invalid-save-html", message: urlValidation.error.message }), ); @@ -999,6 +1002,7 @@ export function initQueueRoutes(deps: QueueDependencies): Router { const title = titlePart ? titlePart.content.toString("utf8") : undefined; if (!contentBytes || contentBytes.length === 0) { + emitSaveIntent({ req, url: submittedUrl, path: SAVE_INTENT_PATH.saveContent, surface: SAVE_SURFACES.extension, outcome: SAVE_OUTCOMES.error }); res.status(422).type(SIREN_MEDIA_TYPE).json( sirenError({ code: "invalid-save-content", @@ -1010,6 +1014,7 @@ export function initQueueRoutes(deps: QueueDependencies): Router { } if (!mediaType) { + emitSaveIntent({ req, url: submittedUrl, path: SAVE_INTENT_PATH.saveContent, surface: SAVE_SURFACES.extension, outcome: SAVE_OUTCOMES.error }); res.status(422).type(SIREN_MEDIA_TYPE).json( sirenError({ code: "invalid-save-content", @@ -1022,6 +1027,7 @@ export function initQueueRoutes(deps: QueueDependencies): Router { const validation = deps.validateSaveableUrl(submittedUrl); if (validation.status === "ERROR") { + emitSaveIntent({ req, url: submittedUrl, path: SAVE_INTENT_PATH.saveContent, surface: SAVE_SURFACES.extension, outcome: SAVE_OUTCOMES.error }); res.status(422).type(SIREN_MEDIA_TYPE).json( sirenError({ code: "invalid-save-content", @@ -1039,6 +1045,7 @@ export function initQueueRoutes(deps: QueueDependencies): Router { const handler = saveContentHandlers[normalized]; if (!handler) { + emitSaveIntent({ req, url: articleUrl, path: SAVE_INTENT_PATH.saveContent, surface: SAVE_SURFACES.extension, outcome: SAVE_OUTCOMES.error }); res.status(422).type(SIREN_MEDIA_TYPE).json( sirenError({ code: "unsupported-media-type", @@ -1051,6 +1058,7 @@ export function initQueueRoutes(deps: QueueDependencies): Router { const handlerResult = await handler({ url: articleUrl, bytes: contentBytes, title, userId }); if (!handlerResult.ok) { + emitSaveIntent({ req, url: articleUrl, path: SAVE_INTENT_PATH.saveContent, surface: SAVE_SURFACES.extension, outcome: SAVE_OUTCOMES.error }); res.status(422).type(SIREN_MEDIA_TYPE).json( sirenError({ code: handlerResult.code, @@ -1082,6 +1090,7 @@ export function initQueueRoutes(deps: QueueDependencies): Router { const validation = deps.validateSaveableUrl(submittedUrl); if (validation.status === "ERROR") { + emitSaveIntent({ req, url: submittedUrl, path: SAVE_INTENT_PATH.save, surface: SAVE_SURFACES.queueSaveBar, outcome: SAVE_OUTCOMES.error }); const urlState = parseQueueUrl({}); const result = await deps.findArticlesByUser({ userId }); const unreadCount = await deps.countArticlesByUser({ userId, status: "unread" }); diff --git a/projects/hutch/src/runtime/web/pages/queue/queue.save-intent.route.test.ts b/projects/hutch/src/runtime/web/pages/queue/queue.save-intent.route.test.ts index c3d087a7f..b20d0a9eb 100644 --- a/projects/hutch/src/runtime/web/pages/queue/queue.save-intent.route.test.ts +++ b/projects/hutch/src/runtime/web/pages/queue/queue.save-intent.route.test.ts @@ -96,6 +96,40 @@ describe("view_save_intent — authenticated save surfaces", () => { expect(response.headers.location).toBe("/queue?error_code=save_failed"); expect(saveIntents(harness)[0]).toMatchObject({ surface: "queue_save_bar", outcome: "error", is_authenticated: 1 }); }); + + it("emits queue_save_bar / error with a null host when the URL is malformed (422)", async () => { + const harness = useApp(createDefaultTestAppFixture(TEST_APP_ORIGIN)); + const agent = await loginAgent(harness.server, harness.auth); + + const response = await agent.post("/queue/save").type("form").send({ url: "not-a-url" }); + + expect(response.status).toBe(422); + const intents = saveIntents(harness); + assert.equal(intents.length, 1, "exactly one view_save_intent"); + expect(intents[0]).toMatchObject({ + path: "/queue/save", + surface: "queue_save_bar", + outcome: "error", + article_host: null, + content_class: null, + is_authenticated: 1, + }); + }); + + it("emits queue_save_bar / error keeping the parseable host of an unsaveable private-network URL", async () => { + const harness = useApp(createDefaultTestAppFixture(TEST_APP_ORIGIN)); + const agent = await loginAgent(harness.server, harness.auth); + + const response = await agent.post("/queue/save").type("form").send({ url: "http://localhost/x" }); + + expect(response.status).toBe(422); + expect(saveIntents(harness)[0]).toMatchObject({ + surface: "queue_save_bar", + outcome: "error", + article_host: "localhost", + content_class: "third_party", + }); + }); }); describe("POST /queue (extension save-article)", () => { @@ -132,6 +166,22 @@ describe("view_save_intent — authenticated save surfaces", () => { expect(response.status).toBe(500); expect(saveIntents(harness)[0]).toMatchObject({ surface: "extension", outcome: "error" }); }); + + it("emits extension / error when the URL is malformed (422)", async () => { + const harness = useApp(createDefaultTestAppFixture(TEST_APP_ORIGIN)); + const token = await bearerToken(harness); + + const response = await request(harness.server) + .post("/queue") + .set("Accept", SIREN_MEDIA_TYPE) + .set("Authorization", `Bearer ${token}`) + .send({ url: "not-a-url" }); + + expect(response.status).toBe(422); + const intents = saveIntents(harness); + assert.equal(intents.length, 1, "exactly one view_save_intent"); + expect(intents[0]).toMatchObject({ path: "/queue", surface: "extension", outcome: "error", article_host: null }); + }); }); describe("POST /queue/save-html (extension)", () => { @@ -162,6 +212,34 @@ describe("view_save_intent — authenticated save surfaces", () => { expect(response.status).toBe(500); expect(saveIntents(harness)[0]).toMatchObject({ path: "/queue/save-html", surface: "extension", outcome: "error" }); }); + + it("emits extension / error when the request fails schema validation (invalid url, 422)", async () => { + const harness = useApp(createDefaultTestAppFixture(TEST_APP_ORIGIN)); + const token = await bearerToken(harness); + + const response = await request(harness.server) + .post("/queue/save-html") + .set("Accept", SIREN_MEDIA_TYPE) + .set("Authorization", `Bearer ${token}`) + .send({ url: "not-a-url", rawHtml: "captured" }); + + expect(response.status).toBe(422); + expect(saveIntents(harness)[0]).toMatchObject({ path: "/queue/save-html", surface: "extension", outcome: "error", article_host: null }); + }); + + it("emits extension / error when a schema-valid URL is unsaveable (private network, 422)", async () => { + const harness = useApp(createDefaultTestAppFixture(TEST_APP_ORIGIN)); + const token = await bearerToken(harness); + + const response = await request(harness.server) + .post("/queue/save-html") + .set("Accept", SIREN_MEDIA_TYPE) + .set("Authorization", `Bearer ${token}`) + .send({ url: "http://localhost/x", rawHtml: "captured" }); + + expect(response.status).toBe(422); + expect(saveIntents(harness)[0]).toMatchObject({ path: "/queue/save-html", surface: "extension", outcome: "error", article_host: "localhost" }); + }); }); describe("POST /queue/save-content (extension)", () => { @@ -196,6 +274,84 @@ describe("view_save_intent — authenticated save surfaces", () => { expect(response.status).toBe(500); expect(saveIntents(harness)[0]).toMatchObject({ surface: "extension", outcome: "error" }); }); + + it("emits extension / error when the URL is malformed (422)", async () => { + const harness = useApp(createDefaultTestAppFixture(TEST_APP_ORIGIN)); + const token = await bearerToken(harness); + + const response = await request(harness.server) + .post("/queue/save-content") + .set("Accept", SIREN_MEDIA_TYPE) + .set("Authorization", `Bearer ${token}`) + .field("url", "not-a-url") + .field("mediaType", "text/html") + .attach("content", Buffer.from("captured"), "content"); + + expect(response.status).toBe(422); + expect(saveIntents(harness)[0]).toMatchObject({ path: "/queue/save-content", surface: "extension", outcome: "error", article_host: null }); + }); + + it("emits extension / error when the content field is missing (422)", async () => { + const harness = useApp(createDefaultTestAppFixture(TEST_APP_ORIGIN)); + const token = await bearerToken(harness); + + const response = await request(harness.server) + .post("/queue/save-content") + .set("Accept", SIREN_MEDIA_TYPE) + .set("Authorization", `Bearer ${token}`) + .field("url", "https://example.com/article") + .field("mediaType", "text/html"); + + expect(response.status).toBe(422); + expect(saveIntents(harness)[0]).toMatchObject({ path: "/queue/save-content", surface: "extension", outcome: "error", article_host: "example.com" }); + }); + + it("emits extension / error when the mediaType field is missing (422)", async () => { + const harness = useApp(createDefaultTestAppFixture(TEST_APP_ORIGIN)); + const token = await bearerToken(harness); + + const response = await request(harness.server) + .post("/queue/save-content") + .set("Accept", SIREN_MEDIA_TYPE) + .set("Authorization", `Bearer ${token}`) + .field("url", "https://example.com/article") + .attach("content", Buffer.from("captured"), "content"); + + expect(response.status).toBe(422); + expect(saveIntents(harness)[0]).toMatchObject({ path: "/queue/save-content", surface: "extension", outcome: "error", article_host: "example.com" }); + }); + + it("emits extension / error on an unsupported media type (422)", async () => { + const harness = useApp(createDefaultTestAppFixture(TEST_APP_ORIGIN)); + const token = await bearerToken(harness); + + const response = await request(harness.server) + .post("/queue/save-content") + .set("Accept", SIREN_MEDIA_TYPE) + .set("Authorization", `Bearer ${token}`) + .field("url", "https://example.com/article") + .field("mediaType", "application/xml") + .attach("content", Buffer.from("hi"), "content"); + + expect(response.status).toBe(422); + expect(saveIntents(harness)[0]).toMatchObject({ path: "/queue/save-content", surface: "extension", outcome: "error", article_host: "example.com" }); + }); + + it("emits extension / error when the uploaded bytes are not a PDF (422)", async () => { + const harness = useApp(createDefaultTestAppFixture(TEST_APP_ORIGIN)); + const token = await bearerToken(harness); + + const response = await request(harness.server) + .post("/queue/save-content") + .set("Accept", SIREN_MEDIA_TYPE) + .set("Authorization", `Bearer ${token}`) + .field("url", "https://example.com/article") + .field("mediaType", "application/pdf") + .attach("content", Buffer.from("not a pdf"), "content"); + + expect(response.status).toBe(422); + expect(saveIntents(harness)[0]).toMatchObject({ path: "/queue/save-content", surface: "extension", outcome: "error", article_host: "example.com" }); + }); }); describe("POST /queue/save-articles (extension bulk)", () => { diff --git a/projects/save-link/src/runtime/domain/generate-summary/generate-summary-handler.test.ts b/projects/save-link/src/runtime/domain/generate-summary/generate-summary-handler.test.ts index 3ef561965..bd7e3aa11 100644 --- a/projects/save-link/src/runtime/domain/generate-summary/generate-summary-handler.test.ts +++ b/projects/save-link/src/runtime/domain/generate-summary/generate-summary-handler.test.ts @@ -46,6 +46,20 @@ function pendingArticle(url: string): Article { }; } +function crawlFailedArticle(url: string): Article { + return { + ...pendingArticle(url), + crawl: { kind: "failed", reason: { kind: "fetch-failed" } }, + }; +} + +function crawlUnsupportedArticle(url: string): Article { + return { + ...pendingArticle(url), + crawl: { kind: "unsupported", reason: { kind: "paywall" } }, + }; +} + type HandlerDeps = Parameters[0]; const NOW = new Date("2026-05-30T12:00:00.000Z"); @@ -227,6 +241,53 @@ describe("initGenerateSummaryHandler", () => { ); }); + it("marks the summary skipped (reason='crawl-failed') when content is absent and the crawl terminally failed, instead of DLQing forever", async () => { + const URL = "https://example.com/crawl-failed"; + const { handler, deps } = createHandler({ + findArticleContent: jest.fn, Parameters>().mockResolvedValue(undefined), + loadArticle: jest.fn().mockResolvedValue(crawlFailedArticle(URL)), + }); + + const result = await handler(createSqsEvent({ url: URL }), buildLambdaContext(), () => {}); + + expect(result).toEqual({ batchItemFailures: [] }); + expect(deps.transitionAndPersist).toHaveBeenCalledWith(markSummarySkipped, { + url: URL, + input: { reason: "crawl-failed", now: NOW.toISOString() }, + }); + expect(deps.summarizeArticle).not.toHaveBeenCalled(); + }); + + it("marks the summary skipped (reason='crawl-unsupported') when content is absent and the crawl is unsupported", async () => { + const URL = "https://example.com/crawl-unsupported"; + const { handler, deps } = createHandler({ + findArticleContent: jest.fn, Parameters>().mockResolvedValue(undefined), + loadArticle: jest.fn().mockResolvedValue(crawlUnsupportedArticle(URL)), + }); + + const result = await handler(createSqsEvent({ url: URL }), buildLambdaContext(), () => {}); + + expect(result).toEqual({ batchItemFailures: [] }); + expect(deps.transitionAndPersist).toHaveBeenCalledWith(markSummarySkipped, { + url: URL, + input: { reason: "crawl-unsupported", now: NOW.toISOString() }, + }); + expect(deps.summarizeArticle).not.toHaveBeenCalled(); + }); + + it("keeps the assert → DLQ path when content is absent and no aggregate row exists (unknown row, not a terminal crawl)", async () => { + const URL = "https://example.com/no-row"; + const { handler, deps } = createHandler({ + findArticleContent: jest.fn, Parameters>().mockResolvedValue(undefined), + loadArticle: jest.fn().mockResolvedValue(undefined), + }); + + const result = await handler(createSqsEvent({ url: URL }), buildLambdaContext(), () => {}); + + expect(result).toEqual({ batchItemFailures: [{ itemIdentifier: "msg-1" }] }); + expect(deps.transitionAndPersist).not.toHaveBeenCalled(); + }); + it("reports batchItemFailures on invalid command schema (Zod failure)", async () => { const { handler, deps } = createHandler(); diff --git a/projects/save-link/src/runtime/domain/generate-summary/generate-summary-handler.ts b/projects/save-link/src/runtime/domain/generate-summary/generate-summary-handler.ts index 63e30080c..e5887781a 100644 --- a/projects/save-link/src/runtime/domain/generate-summary/generate-summary-handler.ts +++ b/projects/save-link/src/runtime/domain/generate-summary/generate-summary-handler.ts @@ -50,6 +50,30 @@ export function initGenerateSummaryHandler(deps: GenerateSummaryHandlerDeps): Ha } const article = await findArticleContent(command.url); + /* Terminal crawl without canonical content is not a race — the crawl + * gave up, so no content will ever arrive. Collapse the summary to + * skipped (the same terminal outcome markCrawlBlocked/markCrawlNotFound + * write) so the auto-heal loop stops repriming this row forever. A + * pending crawl (content still landing) or a ready crawl (content + * genuinely missing) keeps the assert → retry → DLQ path below. */ + if ( + !article && + existing && + (existing.crawl.kind === "failed" || existing.crawl.kind === "unsupported") + ) { + await transitionAndPersist(markSummarySkipped, { + url: command.url, + input: { + reason: existing.crawl.kind === "failed" ? "crawl-failed" : "crawl-unsupported", + now: now().toISOString(), + }, + }); + logger.info("[GenerateSummary] crawl terminal without canonical content — marking summary skipped", { + url: command.url, + crawl: existing.crawl.kind, + }); + continue; + } assert(article, `Article content not found: ${command.url}`); const result = await summarizeArticle({ diff --git a/src/packages/web-analytics/src/analytics.test.ts b/src/packages/web-analytics/src/analytics.test.ts index 5a4ab0c71..aadffd76b 100644 --- a/src/packages/web-analytics/src/analytics.test.ts +++ b/src/packages/web-analytics/src/analytics.test.ts @@ -401,6 +401,11 @@ describe("buildSaveIntentEvent", () => { expect(event).toMatchObject({ article_host: "example.com", content_class: "third_party", referrer_host: "readplace.com" }); }); + it("leaves article_host and content_class null when the submitted URL does not parse — save surfaces emit view_save_intent even on a URL-validation failure, where the submitted string is not a saveable URL and there is no host to classify", () => { + const event = buildIntent({ url: "not a url" }); + expect(event).toMatchObject({ article_host: null, content_class: null }); + }); + it("includes pending_save_id when the anonymous prompted-to-sign-up flow threads one, so the later signup joins back to this intent", () => { const event = buildIntent({ pendingSaveId: "pending-abc-123" }); expect(event).toMatchObject({ pending_save_id: "pending-abc-123" }); diff --git a/src/packages/web-analytics/src/analytics.ts b/src/packages/web-analytics/src/analytics.ts index 021c2b0be..dca3ce798 100644 --- a/src/packages/web-analytics/src/analytics.ts +++ b/src/packages/web-analytics/src/analytics.ts @@ -12,7 +12,7 @@ import { STREAMS, } from "./events"; import { - articleHostFrom, + articleHostFromSubmitted, classifyContentSource, type ContentClass, } from "./content-source"; @@ -205,8 +205,8 @@ export interface ViewSaveIntentEvent { event: typeof ANALYTICS_EVENTS.viewSaveIntent; timestamp: string; path: string; - article_host: string; - content_class: ContentClass; + article_host: string | null; + content_class: ContentClass | null; surface: SaveSurface; outcome: SaveOutcome; referrer_host?: string; @@ -332,7 +332,7 @@ export function buildSaveIntentEvent( }, ): ViewSaveIntentEvent { assert(params.req.visitorId, "visitor-id middleware must run before a save surface emits view_save_intent"); - const articleHost = articleHostFrom(params.url); + const articleHost = articleHostFromSubmitted(params.url); const referrerHost = extractReferrerHost(params.req); return { stream: STREAMS.analytics, @@ -340,7 +340,7 @@ export function buildSaveIntentEvent( timestamp: deps.now().toISOString(), path: params.path, article_host: articleHost, - content_class: classifyContentSource(articleHost), + content_class: articleHost === null ? null : classifyContentSource(articleHost), surface: params.surface, outcome: params.outcome, ...(referrerHost ? { referrer_host: referrerHost } : {}), diff --git a/src/packages/web-analytics/src/content-source.test.ts b/src/packages/web-analytics/src/content-source.test.ts index 4830c9711..bab3a7b9f 100644 --- a/src/packages/web-analytics/src/content-source.test.ts +++ b/src/packages/web-analytics/src/content-source.test.ts @@ -1,4 +1,9 @@ -import { articleHostFrom, classifyContentSource, OWN_CONTENT_DOMAINS } from "./content-source"; +import { + articleHostFrom, + articleHostFromSubmitted, + classifyContentSource, + OWN_CONTENT_DOMAINS, +} from "./content-source"; describe("articleHostFrom", () => { it("returns the lowercased hostname so view_opened and view_save_intent normalize identically", () => { @@ -6,6 +11,24 @@ describe("articleHostFrom", () => { }); }); +describe("articleHostFromSubmitted", () => { + it("returns the lowercased hostname for a valid URL", () => { + expect(articleHostFromSubmitted("https://EN.Wikipedia.ORG/wiki/Foo")).toBe("en.wikipedia.org"); + }); + + it("returns null for an unparseable URL", () => { + expect(articleHostFromSubmitted("not a url")).toBeNull(); + }); + + it("returns null for an empty string", () => { + expect(articleHostFromSubmitted("")).toBeNull(); + }); + + it("returns null when the parsed URL has an empty host", () => { + expect(articleHostFromSubmitted("file:///etc/hosts")).toBeNull(); + }); +}); + describe("classifyContentSource", () => { it("classifies an own apex domain as own", () => { for (const domain of OWN_CONTENT_DOMAINS) { diff --git a/src/packages/web-analytics/src/content-source.ts b/src/packages/web-analytics/src/content-source.ts index 3a547f9d9..2e3f27b29 100644 --- a/src/packages/web-analytics/src/content-source.ts +++ b/src/packages/web-analytics/src/content-source.ts @@ -23,6 +23,22 @@ export function articleHostFrom(url: string): string { return new URL(url).hostname; } +/** + * The article's own host, tolerant of unparseable input. Save surfaces emit + * `view_save_intent` even for URL-validation failures, where the submitted + * string may not parse as a URL — this returns `null` for those so the event + * can still record the failure with `article_host`/`content_class` unset, + * rather than throwing and losing the emission. + */ +export function articleHostFromSubmitted(url: string): string | null { + try { + const host = new URL(url).hostname; + return host.length > 0 ? host : null; + } catch { + return null; + } +} + export function classifyContentSource(articleHost: string): ContentClass { const host = articleHost.toLowerCase(); const owned = OWN_CONTENT_DOMAINS.some((domain) => host === domain || host.endsWith(`.${domain}`));