From 45e91f097cdfcb7c7cb7ed879dca47a5a207d856 Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Mon, 2 Nov 2026 15:26:28 -0600 Subject: [PATCH] fix(inbox): requalify existing unread fix (#18670) ## Thinking Path > - Paperclip helps people manage AI agents and their work. > - The personal inbox must keep new comments unread until the person reads or answers them. > - A comment updates task recency, even when another person writes it. > - The assignee unread query treats that task timestamp as a read receipt. > - [rudycelekli](https://github.com/rudycelekli) already repaired this in #15085, including a reviewed deleted-comment safeguard. > - That PR now has merge conflicts and checks from an older base. This PR ports and requalifies the existing unread fix on current master. > - The port adds real human-comment and API regressions. The original contribution remains open and unchanged. ## Linked Issues or Issue Description Fixes: #15079 Refs: #15085 This is an attributed maintainer port of #15085 at `fe6a486c9ca21cd3a6ea0cef9764c372c53f4572`. The commit includes the verified original contributor credit. This is not an independent claim to the original repair. ## What Changed - Port the stable creation-time fallback for a human assignee in SQL and derived user context. Explicit reads and own replies still take precedence. - Port the reviewed safeguard that excludes deleted external comments. Keep the historical touch from a deleted own reply. - Exercise real comment writes, task recency updates, existing read receipts, read/unread toggles, later assignment, company isolation, and comment deletion through native PostgreSQL and ordinary API projections. ## Verification - Native PostgreSQL regressions fail before the repair: real comment writes leave the assignee falsely read, and later assignment hides existing comments. - A separate deletion regression fails before the reviewed safeguard: a deleted external comment remains the latest external activity. - The frozen head `0dadf85611cbfdc7a762cb82b03ed64519be5434` passes all 41 focused inbox, source-visibility, request, archive, and user-context tests on native PostgreSQL. No tests were skipped. - `pnpm -r typecheck` and `pnpm build` pass on that same frozen head with Node 24 and pnpm 9.15.4. The current-head CI is green, including all general and serialized test shards, build, typecheck, Runner checks, and eight E2E shards. [CI run](https://github.com/paperclipai/paperclip/actions/runs/41525545915). - The local full-test attempt was stopped before the one master rebase. It is incomplete and is not a local full-suite pass. The full current-head CI completed successfully. - Fresh Greptile review is 5/5 on `0dadf85611cbfdc7a762cb82b03ed64519be5434`, with no actionable findings or open review threads. ## Risks - Assigned tasks with older external comments can appear unread. A later explicit read or own reply still clears them. - This changes no schema, API fields, company membership rules, or query joins. - Legacy compact-response cache policy is unchanged. Immediate mutation checks use the ordinary, rows, and index projections. ## Model Used OpenAI Codex, based on GPT-6, with reasoning, repository inspection, code editing, and command execution. This session does not expose a more specific backend model identifier or context-window size. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes (no documentation change is needed to restore the existing unread contract) - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge Co-authored-by: Rudy Celekli <47457359+rudycelekli@users.noreply.github.com> Co-authored-by: Paperclip --- .../__tests__/issues-assignee-unread.test.ts | 110 ++++++++++++++++++ .../src/__tests__/issues-user-context.test.ts | 6 +- server/src/services/issues.ts | 10 +- 3 files changed, 120 insertions(+), 6 deletions(-) create mode 100644 server/src/__tests__/issues-assignee-unread.test.ts diff --git a/server/src/__tests__/issues-assignee-unread.test.ts b/server/src/__tests__/issues-assignee-unread.test.ts new file mode 100644 index 00000000000..ab1c0fb9966 --- /dev/null +++ b/server/src/__tests__/issues-assignee-unread.test.ts @@ -0,0 +1,110 @@ +import { randomUUID } from "node:crypto"; +import { eq, sql } from "drizzle-orm"; +import request from "supertest"; +import { expect, it } from "vitest"; +import { activityLog, agents, authUsers, issues } from "@paperclipai/db"; +import { issueRoutes } from "../routes/issues.js"; +import { deriveIssueUserContext, issueService } from "../services/issues.js"; +import { describeEmbeddedPostgres, resetCompanyIssueFixtures, routeApp, seedCompanyWithBoardAccess, useEmbeddedPostgres } from "./helpers/route-test-harness.js"; + +describeEmbeddedPostgres("human assignee unread comments", () => { + const ctx = useEmbeddedPostgres("paperclip-assignee-unread-", { + resetEach: async db => { + await db.delete(activityLog); + await db.delete(issues); + await db.delete(agents); + await resetCompanyIssueFixtures(db); + await db.delete(authUsers); + }, + }); + + it.each([false, true])("keeps another human's real comment unread through recency updates (prior read: %s)", async priorRead => { + const { companyId, userId, actor } = await seedCompanyWithBoardAccess(ctx.db, "Assigned human task"); + const otherCompany = await seedCompanyWithBoardAccess(ctx.db, "Unrelated company"); + const svc = issueService(ctx.db), app = routeApp(ctx.db, actor, issueRoutes); + const issue = await svc.create(companyId, { + title: "Human inbox comment", status: "in_progress", createdByUserId: userId, assigneeUserId: userId, + }); + if (priorRead) await request(app).post(`/api/issues/${issue.id}/read`).send({}).expect(200); + const comment = await svc.addComment(issue.id, "Another human supplied an update", { userId: `author-${randomUUID()}` }); + const [afterComment] = await ctx.db.select().from(issues).where(eq(issues.id, issue.id)); + // Exercise the real insert and recency update. No historical timestamps or + // mocked comment state hide the ordering that used to clear unread status. + expect(afterComment!.updatedAt.getTime()).toBeGreaterThanOrEqual(new Date(comment.createdAt).getTime()); + const path = `/api/companies/${companyId}/issues`; + const expectUnread = async (unread: boolean) => { + for (const projection of ["", "&view=rows", "&view=index"]) { + const result = await request(app).get(`${path}?q=${encodeURIComponent(issue.title)}&touchedByUserId=me&limit=100${projection}`).expect(200); + expect(result.body).toHaveLength(1); + expect(result.body[0].isUnreadForMe).toBe(unread); + const filtered = await request(app).get(`${path}?q=${encodeURIComponent(issue.title)}&unreadForUserId=me&limit=100${projection}`).expect(200); + expect(filtered.body.map((row: { id: string }) => row.id)).toEqual(unread ? [issue.id] : []); + } + expect(await svc.countUnreadTouchedByUser(companyId, userId)).toBe(unread ? 1 : 0); + expect(await svc.unreadInboxIssueIds(companyId, userId, sql`true`)).toEqual(unread ? [issue.id] : []); + }; + await expectUnread(true); + // Legacy compact responses have a short TTL. Check their SQL projection + // once; immediate mutation readback belongs to ordinary/rows/index. + const compact = await request(app).get(`${path}?q=${encodeURIComponent(issue.title)}&touchedByUserId=me&view=compact`).expect(200); + expect(compact.body[0].isUnreadForMe).toBe(true); + const derived = deriveIssueUserContext(afterComment!, userId, { + myLastCommentAt: null, myLastReadAt: null, lastExternalCommentAt: comment.createdAt, + }); + expect(derived.isUnreadForMe).toBe(true); + await request(app).post(`/api/issues/${issue.id}/read`).send({}).expect(200); + await expectUnread(false); + await request(app).delete(`/api/issues/${issue.id}/read`).expect(200); + await expectUnread(true); + await svc.addComment(issue.id, "I have read this update", { userId }); + await expectUnread(false); + await request(app).get(`/api/companies/${otherCompany.companyId}/issues?unreadForUserId=me`).expect(403); + expect(await svc.countUnreadTouchedByUser(otherCompany.companyId, userId)).toBe(0); + }); + + it("shows existing comments as unread when a task is subsequently assigned to a human", async () => { + const { companyId, userId, actor } = await seedCompanyWithBoardAccess(ctx.db, "New assignment"); + const svc = issueService(ctx.db), app = routeApp(ctx.db, actor, issueRoutes); + const issue = await svc.create(companyId, { title: "Assigned after discussion", status: "backlog" }); + await svc.addComment(issue.id, "Context written before assignment", { userId: `author-${randomUUID()}` }); + await request(app).patch(`/api/issues/${issue.id}`).send({ assigneeUserId: userId, status: "in_progress" }).expect(200); + const result = await request(app).get(`/api/companies/${companyId}/issues?q=${encodeURIComponent(issue.title)}&unreadForUserId=me`).expect(200); + expect(result.body.map((row: { id: string }) => row.id)).toEqual([issue.id]); + expect(result.body[0].isUnreadForMe).toBe(true); + await request(app).post(`/api/issues/${issue.id}/read`).send({}).expect(200); + expect(await svc.countUnreadTouchedByUser(companyId, userId)).toBe(0); + }); + + it("excludes deleted external comments but retains the historical touch from a deleted own reply", async () => { + const { companyId, userId } = await seedCompanyWithBoardAccess(ctx.db, "Deleted comment controls"); + const svc = issueService(ctx.db); + const issue = await svc.create(companyId, { + title: "Deleted inbox activity", status: "in_progress", assigneeUserId: userId, + }); + const authorId = `author-${randomUUID()}`; + const first = await svc.addComment(issue.id, "Keep this live update", { userId: authorId }); + const second = await svc.addComment(issue.id, "Remove this later update", { userId: authorId }); + const observation = async () => { + const [row] = await svc.list(companyId, { touchedByUserId: userId }); + return { row, count: await svc.countUnreadTouchedByUser(companyId, userId) }; + }; + await svc.tombstoneComment(second.id, { actorType: "user", userId }); + expect((await observation()).count).toBe(1); + expect((await observation()).row!.lastExternalCommentAt).toEqual(new Date(first.createdAt)); + await svc.tombstoneComment(first.id, { actorType: "user", userId }); + const removed = await observation(); + expect(removed.count).toBe(0); + expect(removed.row!.isUnreadForMe).toBe(false); + expect(removed.row!.lastExternalCommentAt).toBeNull(); + await svc.addComment(issue.id, "New live activity", { userId: authorId }); + expect((await observation()).count).toBe(1); + const ownReply = await svc.addComment(issue.id, "I saw the live update", { userId }); + expect((await observation()).count).toBe(0); + await svc.tombstoneComment(ownReply.id, { actorType: "user", userId }); + const seen = await observation(); + expect(seen.count).toBe(0); + expect(seen.row!.isUnreadForMe).toBe(false); + expect(seen.row!.myLastTouchAt).toEqual(new Date(ownReply.createdAt)); + }); + +}); diff --git a/server/src/__tests__/issues-user-context.test.ts b/server/src/__tests__/issues-user-context.test.ts index 421934ee270..568348e5ae9 100644 --- a/server/src/__tests__/issues-user-context.test.ts +++ b/server/src/__tests__/issues-user-context.test.ts @@ -76,7 +76,7 @@ describe("deriveIssueUserContext", () => { } }); - it("uses issue updated time as fallback touch point for assignee", () => { + it("uses stable creation time for assignees instead of treating task recency as a read", () => { const context = deriveIssueUserContext( makeIssue({ assigneeUserId: "user-1", updatedAt: new Date("2026-03-06T15:00:00.000Z") }), "user-1", @@ -87,8 +87,8 @@ describe("deriveIssueUserContext", () => { }, ); - expect(context.myLastTouchAt?.toISOString()).toBe("2026-03-06T15:00:00.000Z"); - expect(context.isUnreadForMe).toBe(false); + expect(context.myLastTouchAt?.toISOString()).toBe("2026-03-06T10:00:00.000Z"); + expect(context.isUnreadForMe).toBe(true); }); it("uses latest read timestamp to clear unread without requiring a comment", () => { diff --git a/server/src/services/issues.ts b/server/src/services/issues.ts index 6175f0f15dc..2e1bcd372cf 100644 --- a/server/src/services/issues.ts +++ b/server/src/services/issues.ts @@ -2947,7 +2947,7 @@ function myLastTouchAtExpr(companyId: string, userId: string) { COALESCE(${myLastCommentAt}, to_timestamp(0)), COALESCE(${myLastReadAt}, to_timestamp(0)), COALESCE(CASE WHEN ${issues.createdByUserId} = ${userId} AND ${humanCreatedIssueCondition()} THEN ${issues.createdAt} ELSE NULL END, to_timestamp(0)), - COALESCE(CASE WHEN ${issues.assigneeUserId} = ${userId} THEN ${issues.updatedAt} ELSE NULL END, to_timestamp(0)) + COALESCE(CASE WHEN ${issues.assigneeUserId} = ${userId} THEN ${issues.createdAt} ELSE NULL END, to_timestamp(0)) ) `; } @@ -3014,6 +3014,7 @@ function unreadForUserCondition(companyId: string, userId: string) { FROM ${issueComments} WHERE ${issueComments.issueId} = ${issues.id} AND ${issueComments.companyId} = ${companyId} + AND ${issueComments.deletedAt} IS NULL AND ( ${issueComments.authorUserId} IS NULL OR ${issueComments.authorUserId} <> ${userId} @@ -3136,8 +3137,10 @@ export function deriveIssueUserContext( issue.createdByUserId === userId && !isAgentCreatedIssue(issue) ? normalizeDate(issue.createdAt) : null; + // Assignment establishes inbox participation, not a read. Other people's + // comments also advance updatedAt, so only creation is a stable fallback. const assignedTouchAt = - issue.assigneeUserId === userId ? normalizeDate(issue.updatedAt) : null; + issue.assigneeUserId === userId ? normalizeDate(issue.createdAt) : null; const myLastTouchAt = [myLastCommentAt, myLastReadAt, createdTouchAt, assignedTouchAt] .filter((value): value is Date => value instanceof Date) @@ -5151,7 +5154,8 @@ async function userCommentStatsForIssues( lastExternalCommentAt: sql` MAX( CASE - WHEN ${issueComments.authorUserId} IS NULL OR ${issueComments.authorUserId} <> ${userId} + WHEN ${issueComments.deletedAt} IS NULL + AND (${issueComments.authorUserId} IS NULL OR ${issueComments.authorUserId} <> ${userId}) THEN ${issueComments.createdAt} END )