feat(mcp): Add commentCount to document info response (#12355)

* Add commentsCount to MCP document info response

* fix: refine MCP document comment counts
This commit is contained in:
Tom Moor
2026-05-16 08:42:48 -04:00
committed by GitHub
parent 2fb34a400d
commit a5c22bbb09
10 changed files with 391 additions and 11 deletions
@@ -0,0 +1,20 @@
"use strict";
module.exports = {
async up(queryInterface) {
await queryInterface.sequelize.query(`
UPDATE comments AS reply
SET "resolvedAt" = parent."resolvedAt",
"resolvedById" = parent."resolvedById"
FROM comments AS parent
WHERE reply."parentCommentId" = parent.id
AND parent."resolvedAt" IS NOT NULL
AND reply."resolvedAt" IS NULL;
`);
},
async down() {
// No-op: the inherited resolved state on replies is now load-bearing for
// the unresolved commentCount counter cache, so it cannot safely be undone.
},
};
+99
View File
@@ -186,4 +186,103 @@ describe("Comment", () => {
expect(() => comment.unresolve()).toThrow();
});
});
describe("cascade resolved state", () => {
it("propagates resolvedAt to existing replies when the thread is resolved", async () => {
const user = await buildUser();
const document = await buildDocument({
userId: user.id,
teamId: user.teamId,
});
const thread = await buildComment({
userId: user.id,
documentId: document.id,
});
const reply = await buildComment({
userId: user.id,
documentId: document.id,
parentCommentId: thread.id,
});
thread.resolve(user);
await thread.save();
await reply.reload();
expect(reply.resolvedAt).toEqual(thread.resolvedAt);
expect(reply.resolvedById).toEqual(user.id);
});
it("clears resolvedAt on replies when the thread is unresolved", async () => {
const user = await buildUser();
const document = await buildDocument({
userId: user.id,
teamId: user.teamId,
});
const thread = await buildComment({
userId: user.id,
documentId: document.id,
});
const reply = await buildComment({
userId: user.id,
documentId: document.id,
parentCommentId: thread.id,
});
thread.resolve(user);
await thread.save();
thread.unresolve();
await thread.save();
await reply.reload();
expect(reply.resolvedAt).toBeNull();
expect(reply.resolvedById).toBeNull();
});
it("inherits resolved state when a reply is created on a resolved thread", async () => {
const user = await buildUser();
const document = await buildDocument({
userId: user.id,
teamId: user.teamId,
});
const thread = await buildComment({
userId: user.id,
documentId: document.id,
});
thread.resolve(user);
await thread.save();
const reply = await buildComment({
userId: user.id,
documentId: document.id,
parentCommentId: thread.id,
});
expect(reply.resolvedAt).toEqual(thread.resolvedAt);
expect(reply.resolvedById).toEqual(user.id);
});
it("rejects replies to comments in a different document", async () => {
const user = await buildUser();
const document = await buildDocument({
userId: user.id,
teamId: user.teamId,
});
const otherDocument = await buildDocument({
userId: user.id,
teamId: user.teamId,
});
const thread = await buildComment({
userId: user.id,
documentId: otherDocument.id,
});
await expect(
buildComment({
userId: user.id,
documentId: document.id,
parentCommentId: thread.id,
})
).rejects.toThrow("Parent comment must belong to the same document");
});
});
});
+82 -1
View File
@@ -1,20 +1,29 @@
import { Node } from "prosemirror-model";
import type { InferAttributes, InferCreationAttributes } from "sequelize";
import type {
CreateOptions,
InferAttributes,
InferCreationAttributes,
InstanceUpdateOptions,
} from "sequelize";
import {
DataType,
BelongsTo,
BeforeCreate,
ForeignKey,
Column,
Table,
Length,
DefaultScope,
AfterDestroy,
AfterUpdate,
} from "sequelize-typescript";
import type { ProsemirrorData, ReactionSummary } from "@shared/types";
import { ProsemirrorHelper } from "@shared/utils/ProsemirrorHelper";
import { CommentValidation } from "@shared/validations";
import { commentSchema } from "@server/editor";
import { ValidationError } from "@server/errors";
import { CacheHelper } from "@server/utils/CacheHelper";
import { RedisPrefixHelper } from "@server/utils/RedisPrefixHelper";
import Document from "./Document";
import User from "./User";
import { type HookContext } from "./base/Model";
@@ -143,6 +152,78 @@ class Comment extends ParanoidModel<
// hooks
// A reply created on an already-resolved thread inherits the parent's
// resolved state so the resolvedAt column alone can answer "is this thread
// resolved?" — keeping read queries simple and the counter cache index-only.
@BeforeCreate
public static async inheritResolvedFromParent(
model: Comment,
options: CreateOptions<InferAttributes<Comment>>
) {
if (!model.parentCommentId || model.resolvedAt) {
return;
}
const parent = await this.unscoped().findOne({
where: {
id: model.parentCommentId,
documentId: model.documentId,
},
transaction: options.transaction,
lock: options.transaction
? { level: options.transaction.LOCK.UPDATE, of: this }
: undefined,
});
if (!parent) {
throw ValidationError("Parent comment must belong to the same document");
}
if (parent?.resolvedAt) {
model.resolvedAt = parent.resolvedAt;
model.resolvedById = parent.resolvedById;
}
}
// When a thread root is resolved or unresolved, propagate the same state to
// its replies and invalidate the document's commentCount counter cache.
@AfterUpdate
public static async cascadeResolvedToReplies(
model: Comment,
options: InstanceUpdateOptions<InferAttributes<Comment>>
) {
if (!model.changed("resolvedAt")) {
return;
}
if (model.parentCommentId === null) {
await this.update(
{
resolvedAt: model.resolvedAt,
resolvedById: model.resolvedById,
},
{
where: { parentCommentId: model.id, documentId: model.documentId },
transaction: options.transaction,
hooks: false,
}
);
}
const invalidate = () =>
CacheHelper.removeData(
RedisPrefixHelper.getCounterCacheKey(
"Document",
"unresolvedComments",
model.documentId
)
);
if (options.transaction) {
const transaction = options.transaction.parent || options.transaction;
transaction.afterCommit(invalidate);
} else {
await invalidate();
}
}
@AfterDestroy
public static async deleteChildComments(model: Comment, ctx: HookContext) {
const { transaction } = ctx;
+99
View File
@@ -7,6 +7,8 @@ import {
buildDocument,
buildDraftDocument,
buildCollection,
buildComment,
buildResolvedComment,
buildTeam,
buildUser,
buildGuestUser,
@@ -352,3 +354,100 @@ describe("tasks", () => {
expect(newTasks.total).toBe(3);
});
});
describe("commentCount", () => {
it("returns 0 for a document with no comments", async () => {
const document = await buildDocument();
expect(await document.commentCount).toEqual(0);
});
it("counts unresolved threads and their replies", async () => {
const document = await buildDocument();
const thread = await buildComment({
documentId: document.id,
userId: document.createdById,
});
await buildComment({
documentId: document.id,
userId: document.createdById,
parentCommentId: thread.id,
});
expect(await document.commentCount).toEqual(2);
});
it("excludes resolved threads and their replies", async () => {
const document = await buildDocument();
const user = await buildUser({ teamId: document.teamId });
const resolved = await buildResolvedComment(user, {
documentId: document.id,
userId: user.id,
});
await buildComment({
documentId: document.id,
userId: user.id,
parentCommentId: resolved.id,
});
await buildComment({
documentId: document.id,
userId: user.id,
});
expect(await document.commentCount).toEqual(1);
});
it("invalidates the cached count when a comment is destroyed", async () => {
const document = await buildDocument();
const comment = await buildComment({
documentId: document.id,
userId: document.createdById,
});
await buildComment({
documentId: document.id,
userId: document.createdById,
});
expect(await document.commentCount).toEqual(2);
await comment.destroy();
expect(await document.commentCount).toEqual(1);
});
it("invalidates the cached count when a thread is resolved", async () => {
const document = await buildDocument();
const user = await buildUser({ teamId: document.teamId });
const thread = await buildComment({
documentId: document.id,
userId: user.id,
});
await buildComment({
documentId: document.id,
userId: user.id,
parentCommentId: thread.id,
});
// Prime the cache.
expect(await document.commentCount).toEqual(2);
thread.resolve(user);
await thread.save();
expect(await document.commentCount).toEqual(0);
});
it("invalidates the cached count when a resolved thread is unresolved", async () => {
const document = await buildDocument();
const user = await buildUser({ teamId: document.teamId });
const thread = await buildResolvedComment(user, {
documentId: document.id,
userId: user.id,
});
// Prime the cache (resolved thread is excluded).
expect(await document.commentCount).toEqual(0);
thread.unresolve();
await thread.save();
expect(await document.commentCount).toEqual(1);
});
});
+9
View File
@@ -51,6 +51,7 @@ import { DocumentValidation } from "@shared/validations";
import { InvalidRequestError, ValidationError } from "@server/errors";
import { generateUrlId } from "@server/utils/url";
import Collection from "./Collection";
import Comment from "./Comment";
import FileOperation from "./FileOperation";
import Group from "./Group";
import GroupMembership from "./GroupMembership";
@@ -64,6 +65,7 @@ import User from "./User";
import UserMembership from "./UserMembership";
import View from "./View";
import ArchivableModel from "./base/ArchivableModel";
import { CounterCache } from "./decorators/CounterCache";
import Fix from "./decorators/Fix";
import { DocumentHelper } from "./helpers/DocumentHelper";
import IsHexColor from "./validators/IsHexColor";
@@ -678,6 +680,13 @@ class Document extends ArchivableModel<
@HasMany(() => View)
views: View[];
@CounterCache(() => Comment, {
as: "unresolvedComments",
foreignKey: "documentId",
where: { resolvedAt: { [Op.is]: null } },
})
commentCount: Promise<number>;
/**
* Returns an array of unique userIds that are members of a document
* either via group or direct membership.
+6
View File
@@ -18,6 +18,9 @@ type Options = {
includeData?: boolean;
/** Include the updatedAt timestamp for public documents. */
includeUpdatedAt?: boolean;
/** Include the unresolved comment count. Each call triggers a Redis lookup
* so only enable when the consumer needs the signal (e.g. MCP). */
includeCommentCount?: boolean;
/** Array of backlink document IDs to include in the response. */
backlinkIds?: string[];
};
@@ -112,6 +115,9 @@ async function presentDocument(
res.templateId = document.templateId;
res.insightsEnabled = document.insightsEnabled;
res.popularityScore = document.popularityScore;
if (options.includeCommentCount) {
res.commentCount = await document.commentCount;
}
res.sourceMetadata = document.sourceMetadata
? {
importedAt: source?.createdAt ?? document.createdAt,
+3
View File
@@ -35,6 +35,9 @@ describe("list_documents", () => {
(d: { document: { id: string } }) => d.document.id === document.id
) as { document: { url: string } };
expect(match.document.url).toMatch(/^https?:\/\//);
expect(
(match.document as { commentCount?: number }).commentCount
).toBeUndefined();
});
it("filters by collection", async () => {
+30 -7
View File
@@ -8,7 +8,10 @@ import { Op } from "sequelize";
import { Collection, Document } from "@server/models";
import { sequelize } from "@server/storage/database";
import { authorize, can } from "@server/policies";
import { presentDocument, presentNavigationNode } from "@server/presenters";
import {
presentDocument as presentDocumentBase,
presentNavigationNode,
} from "@server/presenters";
import AuthenticationHelper from "@shared/helpers/AuthenticationHelper";
import { UrlHelper } from "@shared/utils/UrlHelper";
import {
@@ -26,6 +29,26 @@ import {
import { TextEditMode } from "@shared/types";
import SearchProviderManager from "@server/utils/SearchProviderManager";
/**
* Presents a document for a tool response. Adds MCP-specific fields
* on top of the standard document presenter.
*
* @param document - the document to present.
* @param options - optional presenter options
* @returns the presented document object.
*/
export function presentDocument(
document: Document,
options: {
includeData?: boolean;
includeText?: boolean;
includeUpdatedAt?: boolean;
includeCommentCount?: boolean;
} = {}
) {
return presentDocumentBase(undefined, document, options);
}
/**
* Registers document-related MCP tools on the given server, filtered by
* the OAuth scopes granted to the current token.
@@ -135,7 +158,7 @@ export function documentTools(server: McpServer, scopes: string[]) {
filteredResults.map(async (result) => {
const doc = pathToUrl(
user.team,
await presentDocument(undefined, result.document, {
await presentDocument(result.document, {
includeData: false,
includeText: false,
})
@@ -156,7 +179,7 @@ export function documentTools(server: McpServer, scopes: string[]) {
if (exactMatch) {
const doc = pathToUrl(
user.team,
await presentDocument(undefined, exactMatch, {
await presentDocument(exactMatch, {
includeData: false,
includeText: false,
})
@@ -199,7 +222,7 @@ export function documentTools(server: McpServer, scopes: string[]) {
documents.map(async (document) => {
const doc = pathToUrl(
user.team,
await presentDocument(undefined, document, {
await presentDocument(document, {
includeData: false,
includeText: false,
})
@@ -353,7 +376,7 @@ export function documentTools(server: McpServer, scopes: string[]) {
});
const [{ text, ...attributes }, breadcrumb] = await Promise.all([
presentDocument(undefined, document, {
presentDocument(document, {
includeData: false,
includeText: true,
includeUpdatedAt: true,
@@ -489,7 +512,7 @@ export function documentTools(server: McpServer, scopes: string[]) {
documents.map(async (document) => {
const doc = pathToUrl(
user.team,
await presentDocument(undefined, document, {
await presentDocument(document, {
includeData: false,
includeText: false,
})
@@ -603,7 +626,7 @@ export function documentTools(server: McpServer, scopes: string[]) {
}
const [{ text, ...attributes }, breadcrumb] = await Promise.all([
presentDocument(undefined, updated, {
presentDocument(updated, {
includeData: false,
includeText: true,
includeUpdatedAt: true,
+40 -1
View File
@@ -1,4 +1,9 @@
import { buildCollection, buildDocument } from "@server/test/factories";
import {
buildCollection,
buildComment,
buildDocument,
buildResolvedComment,
} from "@server/test/factories";
import { getTestServer } from "@server/test/support";
import { buildOAuthUser, callMcpTool } from "@server/test/McpHelper";
@@ -55,4 +60,38 @@ describe("fetch", () => {
// Second content is markdown text
expect(res!.result!.content![1].text).toContain("Hello");
});
it("returns unresolved commentCount on documents", async () => {
const { user, accessToken } = await buildOAuthUser();
const collection = await buildCollection({
teamId: user.teamId,
userId: user.id,
});
const document = await buildDocument({
teamId: user.teamId,
userId: user.id,
collectionId: collection.id,
});
const thread = await buildComment({
documentId: document.id,
userId: user.id,
});
await buildComment({
documentId: document.id,
userId: user.id,
parentCommentId: thread.id,
});
await buildResolvedComment(user, {
documentId: document.id,
userId: user.id,
});
const res = await callMcpTool(server, accessToken, "fetch", {
resource: "document",
id: document.id,
});
const metadata = JSON.parse(res!.result!.content![0].text ?? "{}");
expect(metadata.document.commentCount).toEqual(2);
});
});
+3 -2
View File
@@ -6,11 +6,11 @@ import { authorize, can } from "@server/policies";
import { AuthorizationError } from "@server/errors";
import {
presentCollection,
presentDocument,
presentNavigationNode,
presentUser,
} from "@server/presenters";
import AuthenticationHelper from "@shared/helpers/AuthenticationHelper";
import { presentDocument } from "./documents";
import {
error,
success,
@@ -119,10 +119,11 @@ export function fetchTool(server: McpServer, scopes: string[]) {
authorize(actor, "read", document);
const [{ text, ...attributes }, breadcrumb] = await Promise.all([
presentDocument(undefined, document, {
presentDocument(document, {
includeData: false,
includeText: true,
includeUpdatedAt: true,
includeCommentCount: true,
}),
getDocumentBreadcrumb(document, actor),
]);