diff --git a/packages/mcp/src/client/comments.ts b/packages/mcp/src/client/comments.ts index c1fd557d..df5368e3 100644 --- a/packages/mcp/src/client/comments.ts +++ b/packages/mcp/src/client/comments.ts @@ -59,6 +59,39 @@ import { mergeFootnoteDefinitions, } from "../lib/transforms.js"; +// Max concurrent per-page comment fetches in checkNewComments (#490). The scan is +// O(N) independent REST reads over the working set; running them one-at-a-time made +// a large space linear in round-trips. A small cap parallelizes without hammering +// the server (or exhausting sockets). 6 is a conservative middle of the 5–8 band. +const COMMENT_SCAN_CONCURRENCY = 6; + +/** + * Map `items` through `fn` with at most `limit` in flight, preserving INPUT ORDER + * in the returned array. A tiny bounded pool (no p-limit dependency): `limit` + * workers pull the next index off a shared cursor until the list is drained. + */ +async function mapWithConcurrency( + items: readonly T[], + limit: number, + fn: (item: T, index: number) => Promise, +): Promise { + const results = new Array(items.length); + let cursor = 0; + const worker = async (): Promise => { + for (;;) { + const i = cursor++; + if (i >= items.length) return; + results[i] = await fn(items[i], i); + } + }; + const workers = Array.from( + { length: Math.max(1, Math.min(limit, items.length)) }, + () => worker(), + ); + await Promise.all(workers); + return results; +} + // Public method surface of CommentsMixin (issue #450) — a NAMED type so the factory // return type is expressible in the emitted .d.ts (the anonymous mixin class // carries the base's protected shared state, which would otherwise trip TS4094). @@ -655,27 +688,32 @@ export function CommentsMixin>( parentPageId, ); - // 2. Fetch comments for each page, keep ones created after since - const results: any[] = []; - for (const page of pagesInScope) { - try { - // Full feed (incl. resolved): a "new comments since" scan reports all - // recent activity; the active-only filter is scoped to listComments. - const comments = (await this.listComments(page.id, true)).items; - const newComments = comments.filter( - (c: any) => new Date(c.createdAt) > sinceDate, - ); - if (newComments.length > 0) { - results.push({ - pageId: page.id, - pageTitle: page.title, - comments: newComments, - }); + // 2. Fetch comments for each page, keep ones created after since. Runs with + // bounded concurrency (#490) instead of one-at-a-time — the per-page reads are + // independent, so a large working set no longer costs O(N) serial round-trips. + // Order is preserved (mapWithConcurrency keeps input order), so the output is + // deterministic regardless of which fetch finishes first. + const perPage = await mapWithConcurrency( + pagesInScope, + COMMENT_SCAN_CONCURRENCY, + async (page: any) => { + try { + // Full feed (incl. resolved): a "new comments since" scan reports all + // recent activity; the active-only filter is scoped to listComments. + const comments = (await this.listComments(page.id, true)).items; + const newComments = comments.filter( + (c: any) => new Date(c.createdAt) > sinceDate, + ); + return newComments.length > 0 + ? { pageId: page.id, pageTitle: page.title, comments: newComments } + : null; + } catch (e: any) { + // Skip pages with errors (e.g. deleted between calls) + return null; } - } catch (e: any) { - // Skip pages with errors (e.g. deleted between calls) - } - } + }, + ); + const results: any[] = perPage.filter((r): r is any => r !== null); const totalNewComments = results.reduce( (sum, r) => sum + r.comments.length, diff --git a/packages/mcp/test/mock/pagination-cursor.test.mjs b/packages/mcp/test/mock/pagination-cursor.test.mjs index fd6ab47f..efc997cd 100644 --- a/packages/mcp/test/mock/pagination-cursor.test.mjs +++ b/packages/mcp/test/mock/pagination-cursor.test.mjs @@ -442,3 +442,71 @@ test("checkNewComments subtree includes the root without a separate getPageRaw", assert.equal(result.checkedPages, 2, "root + one descendant scanned"); assert.equal(result.totalNewComments, 1, "the root's fresh comment found"); }); + +// ----------------------------------------------------------------------------- +// 6) checkNewComments parallelism (#490): the per-page comment fetches run with +// bounded concurrency (not one-at-a-time), and the results still preserve the +// page order deterministically regardless of which fetch finishes first. +// ----------------------------------------------------------------------------- +test("checkNewComments fetches pages concurrently (bounded) and preserves order", async () => { + // A subtree with 12 descendants so the scan has plenty to parallelize. + const NODES = [{ id: "parent", title: "Parent", parentPageId: null, hasChildren: true }]; + for (let i = 0; i < 12; i++) { + NODES.push({ id: `k${i}`, title: `Kid ${i}`, parentPageId: "parent", hasChildren: false }); + } + + let inFlight = 0; + let maxInFlight = 0; + + const { baseURL } = await spawn(async (req, res) => { + const raw = await readBody(req); + if (handleLogin(req, res)) return; + if (req.url === "/api/pages/tree") { + sendJson(res, 200, { success: true, data: { items: NODES } }); + return; + } + if (req.url === "/api/comments") { + const body = JSON.parse(raw || "{}"); + inFlight++; + maxInFlight = Math.max(maxInFlight, inFlight); + // Hold the response briefly so concurrent fetches actually overlap. + setTimeout(() => { + inFlight--; + // Every page carries one fresh comment so ordering is observable. + sendJson(res, 200, { + success: true, + data: { + items: [ + { id: `c-${body.pageId}`, createdAt: "2030-01-01T00:00:00.000Z", content: null }, + ], + meta: { nextCursor: null }, + }, + }); + }, 25); + return; + } + sendJson(res, 404, {}); + }); + + const client = new DocmostClient(baseURL, "user@example.com", "pw"); + const result = await client.checkNewComments( + "space-1", + "2020-01-01T00:00:00.000Z", + "parent", + ); + + // 13 pages (parent + 12 kids) were scanned; each had a fresh comment. + assert.equal(result.checkedPages, 13, "all pages scanned"); + assert.equal(result.totalNewComments, 13, "one fresh comment per page"); + // Parallelism: more than one request was in flight at once, but never above the + // cap (6). A serial implementation would show maxInFlight === 1. + assert.ok(maxInFlight > 1, `expected concurrent fetches, saw max ${maxInFlight}`); + assert.ok(maxInFlight <= 6, `concurrency must be bounded, saw ${maxInFlight}`); + // Deterministic order: results follow the page-enumeration order (parent first). + assert.equal(result.comments[0].pageId, "parent", "results preserve page order"); + assert.deepEqual( + result.comments.map((r) => r.pageId), + ["parent", ...Array.from({ length: 12 }, (_, i) => `k${i}`)], + "result order matches the enumeration order regardless of finish order", + ); +});