From 2f3cf402b173d4bc83f2139fd07e289326f96799 Mon Sep 17 00:00:00 2001 From: xarmian Date: Tue, 6 Oct 2026 12:57:14 -0400 Subject: [PATCH] fix(timeline): 'commented on update' only for a comment an update carried (BUG-3437) (#1832) * fix(timeline): 'commented on update' only for a comment an update carried (BUG-3437) TimelineCommentCard showed 'commented on update' whenever the comment had an activity_id, and every comment has one: a comment or reply written on its own links to its own 'commented' activity, so a plain 'pad item comment' read as if it were attached to a change. The store's comment list join (the one that already reads the agent name off the linked activity) now also reads its action into Comment.LinkedActivityAction (json:"-"), and the timeline sets an additive, omitempty TimelineEntry.comment_on_update when that action is anything but 'commented'. The card keys the label on it. The comments API, the CLI and MCP keep their shape (the new field is never serialized outside the timeline, which MCP does not expose). Tests: the server test writes a standalone comment, a reply and an update carrying a comment through the real routes (web, CLI and MCP share the comments route) and checks the flag, with the old rule and an always-false mutant each red; the web test pins the card and the entry list's pass-through, each red with its half reverted. Claude-Session: https://claude.ai/code/session_01Q9WssCjFuHjSfLABoW4tmS * test(timeline): a comment that arrives live keeps 'commented on update' (BUG-3437) The label comes from the server-set comment_on_update. Comments that arrive while the timeline is open come through the SSE refresh, which re-reads the timeline endpoint (nothing builds a comment entry client-side), so they carry the flag. This pins it: red if the entry list stops passing the flag. Claude-Session: https://claude.ai/code/session_01Q9WssCjFuHjSfLABoW4tmS --- internal/models/activity.go | 20 +- internal/models/comment.go | 13 +- .../server/bug3437_comment_on_update_test.go | 88 ++++++++ internal/server/handlers_timeline.go | 7 +- internal/store/comments.go | 7 +- .../timeline/TimelineCommentCard.svelte | 10 +- .../timeline/TimelineEntryList.svelte | 1 + .../timelineCommentOnUpdate.svelte.test.ts | 67 +++++++ ...timelineCommentOnUpdateLive.svelte.test.ts | 189 ++++++++++++++++++ web/src/lib/types/index.ts | 2 + 10 files changed, 388 insertions(+), 16 deletions(-) create mode 100644 internal/server/bug3437_comment_on_update_test.go create mode 100644 web/src/lib/components/timeline/timelineCommentOnUpdate.svelte.test.ts create mode 100644 web/src/lib/components/timeline/timelineCommentOnUpdateLive.svelte.test.ts diff --git a/internal/models/activity.go b/internal/models/activity.go index d8694555b..0425df4ca 100644 --- a/internal/models/activity.go +++ b/internal/models/activity.go @@ -185,13 +185,19 @@ type TimelineEntry struct { // do NOT get it: their name already lives in Activity.Metadata, and a // second copy there would be a second source that can drift (TASK-2760, // lead ruling on the trail). - AgentName string `json:"agent_name,omitempty"` - Source string `json:"source"` - Comment *Comment `json:"comment,omitempty"` - Activity *Activity `json:"activity,omitempty"` - Version *Version `json:"version,omitempty"` - Note *ItemImplementationNote `json:"note,omitempty"` - Decision *ItemDecisionLogEntry `json:"decision,omitempty"` + AgentName string `json:"agent_name,omitempty"` + // CommentOnUpdate is set on a "comment" entry whose comment an item update + // carried (`pad item update --comment`, the update's "updated" activity), + // and false for a comment written on its own, whose linked activity is its + // own "commented" row (BUG-3437). The card's "commented on update" label + // keys on it; it used to key on activity_id, which EVERY comment has. + CommentOnUpdate bool `json:"comment_on_update,omitempty"` + Source string `json:"source"` + Comment *Comment `json:"comment,omitempty"` + Activity *Activity `json:"activity,omitempty"` + Version *Version `json:"version,omitempty"` + Note *ItemImplementationNote `json:"note,omitempty"` + Decision *ItemDecisionLogEntry `json:"decision,omitempty"` // AutosaveRun is set on a collab-snapshot version entry that stands for a // collapsed run of autosaves (PLAN-2348 U3): the entry is the run's // NEWEST row, and this says what the dropped older rows were. diff --git a/internal/models/comment.go b/internal/models/comment.go index cff930e5e..a0d8e18c1 100644 --- a/internal/models/comment.go +++ b/internal/models/comment.go @@ -33,8 +33,17 @@ type Comment struct { Imported bool `json:"imported,omitempty"` // Populated by joins (not stored) - ItemTitle string `json:"item_title,omitempty"` - ItemSlug string `json:"item_slug,omitempty"` + // + // LinkedActivityAction is the action of the activity ActivityID points at, + // read by the same list join as AgentName (BUG-3437): "commented" for a + // comment or reply on its own, "updated" (or another change) for a comment + // an item update carried. Never serialized: the item timeline turns it into + // TimelineEntry.CommentOnUpdate, and nothing else needs it, so the + // comments API and MCP keep their shape. Empty on a comment read any other + // way, and on one with no linked activity. + LinkedActivityAction string `json:"-"` + ItemTitle string `json:"item_title,omitempty"` + ItemSlug string `json:"item_slug,omitempty"` // AgentName is the display name the writing agent declared (the // X-Pad-Agent header), read off the activity this comment's ActivityID // points at — the `commented` row a comment or reply logs, or the diff --git a/internal/server/bug3437_comment_on_update_test.go b/internal/server/bug3437_comment_on_update_test.go new file mode 100644 index 000000000..f0f3786f5 --- /dev/null +++ b/internal/server/bug3437_comment_on_update_test.go @@ -0,0 +1,88 @@ +package server + +import ( + "encoding/json" + "net/http" + "strings" + "testing" +) + +// BUG-3437: the timeline's "commented on update" label keyed on the comment's +// activity_id, and EVERY comment has one: a comment or reply written on its own +// links to its own "commented" activity. The timeline now says which: an +// entry's comment_on_update is true only when an item update carried the +// comment. The web, the CLI (`pad item comment`) and MCP all write a comment +// through the same comments route, so the standalone leg covers all three. +func TestBug3437_CommentOnUpdateOnlyForAnUpdatesComment(t *testing.T) { + t.Parallel() + srv := testServer(t) + ws := createTestWorkspaceViaAPI(t, srv) + item := timelineItemWithStructured(t, srv, ws, "", "") + + standalone := postComment(t, srv, ws, item.Slug, "", "a plain comment") + reply := postReply(t, srv, ws, standalone.ID, "", "a plain reply") + + // An update that carries a comment, the way `pad item update --comment` sends it. + rr := doAttributionRequest(t, srv, "", "PATCH", "/api/v1/workspaces/"+ws+"/items/"+item.Slug, + map[string]any{"title": item.Title + " (renamed)", "comment": "why I renamed it"}) + if rr.Code != http.StatusOK { + t.Fatalf("update with comment: %d %s", rr.Code, rr.Body.String()) + } + var updated struct { + Slug string `json:"slug"` + } + if err := json.Unmarshal(rr.Body.Bytes(), &updated); err != nil || updated.Slug == "" { + t.Fatalf("update response: %v %s", err, rr.Body.String()) + } + item.Slug = updated.Slug // a rename moves the slug + + resp := fetchTimeline(t, srv, ws, item.Slug, "") + var onUpdateID string + for _, e := range resp.Entries { + if e.Kind == "comment" && e.Comment != nil && e.Comment.Body == "why I renamed it" { + onUpdateID = e.ID + if !e.CommentOnUpdate { + t.Errorf("the update's comment: comment_on_update = false, want true") + } + } + } + if onUpdateID == "" { + t.Fatalf("the update's comment is not on the timeline: %+v", resp.Entries) + } + entry := commentEntryByID(resp.Entries, standalone.ID) + if entry == nil { + t.Fatalf("standalone comment not on the timeline") + } + if entry.CommentOnUpdate { + t.Errorf("a standalone comment: comment_on_update = true, want false") + } + if entry.Comment.ActivityID == "" { + t.Errorf("control: the standalone comment has no activity_id, so keying on it would not have failed") + } + // The reply is nested under its parent, and the label is a top-level + // entry's: a reply never becomes an entry with the flag. + for _, r := range entry.Comment.Replies { + if r.ID == reply.ID && r.ActivityID == "" { + t.Errorf("control: the reply has no activity_id") + } + } + + // The comments API and MCP keep their shape: the linked action is never serialized. + rr = doAttributionRequest(t, srv, "", "GET", "/api/v1/workspaces/"+ws+"/items/"+item.Slug+"/comments", nil) + if rr.Code != http.StatusOK { + t.Fatalf("list comments: %d", rr.Code) + } + body := rr.Body.String() + for _, k := range []string{"linked_activity_action", "LinkedActivityAction", "comment_on_update"} { + if strings.Contains(body, k) { + t.Errorf("the comments API now carries %q", k) + } + } + var list []map[string]any + if err := json.Unmarshal(rr.Body.Bytes(), &list); err != nil { + var wrapped map[string][]map[string]any + if err2 := json.Unmarshal(rr.Body.Bytes(), &wrapped); err2 != nil { + t.Fatalf("decode comments: %v / %v", err, err2) + } + } +} diff --git a/internal/server/handlers_timeline.go b/internal/server/handlers_timeline.go index 836765303..cd5c3b99b 100644 --- a/internal/server/handlers_timeline.go +++ b/internal/server/handlers_timeline.go @@ -547,8 +547,11 @@ func buildTimeline(comments []models.Comment, activities []models.Activity, vers // Derived from the nested comment (see TimelineEntry.AgentName); // the store's join is the only writer of the value being copied. AgentName: c.AgentName, - Source: c.Source, - Comment: &comments[i], + // A comment an update carried links to the update's activity; one + // written on its own links to its own "commented" row (BUG-3437). + CommentOnUpdate: c.LinkedActivityAction != "" && c.LinkedActivityAction != "commented", + Source: c.Source, + Comment: &comments[i], } entries = append(entries, entry) } diff --git a/internal/store/comments.go b/internal/store/comments.go index 1ae640bfa..d4a2bec27 100644 --- a/internal/store/comments.go +++ b/internal/store/comments.go @@ -347,7 +347,7 @@ func (s *Store) getCommentQ(q Queryer, id string) (*models.Comment, error) { const commentListCols = `c.id, c.item_id, c.workspace_id, c.author, COALESCE(c.user_id, ''), c.body, c.created_by, c.source, COALESCE(c.activity_id, ''), COALESCE(c.parent_id, ''), c.created_at, c.updated_at, - CASE WHEN c.deleted_at IS NULL THEN 0 ELSE 1 END, c.imported, a.metadata` + CASE WHEN c.deleted_at IS NULL THEN 0 ELSE 1 END, c.imported, a.metadata, a.action` const commentAgentJoin = `LEFT JOIN activities a ON a.id = c.activity_id AND a.document_id = c.item_id` @@ -356,12 +356,12 @@ func scanComments(rows *sql.Rows) ([]models.Comment, error) { for rows.Next() { var c models.Comment var createdAt, updatedAt string - var activityMeta sql.NullString + var activityMeta, activityAction sql.NullString var deleted, imported int if err := rows.Scan( &c.ID, &c.ItemID, &c.WorkspaceID, &c.Author, &c.UserID, &c.Body, &c.CreatedBy, &c.Source, &c.ActivityID, &c.ParentID, - &createdAt, &updatedAt, &deleted, &imported, &activityMeta, + &createdAt, &updatedAt, &deleted, &imported, &activityMeta, &activityAction, ); err != nil { return nil, fmt.Errorf("scan comment: %w", err) } @@ -372,6 +372,7 @@ func scanComments(rows *sql.Rows) ([]models.Comment, error) { if activityMeta.Valid { c.AgentName = models.AgentNameFromMetadata(activityMeta.String) } + c.LinkedActivityAction = activityAction.String comments = append(comments, c) } return comments, rows.Err() diff --git a/web/src/lib/components/timeline/TimelineCommentCard.svelte b/web/src/lib/components/timeline/TimelineCommentCard.svelte index 80bbb2aa8..2d21dd60d 100644 --- a/web/src/lib/components/timeline/TimelineCommentCard.svelte +++ b/web/src/lib/components/timeline/TimelineCommentCard.svelte @@ -10,6 +10,12 @@ interface Props { comment: Comment; + /** + * The comment came with an item update (the timeline entry's + * `comment_on_update`, BUG-3437). Only then is it "commented on + * update": every comment has an activity_id, a standalone one included. + */ + onUpdate?: boolean; wsSlug: string; username?: string; items: Item[]; @@ -57,7 +63,7 @@ onRemoveReaction: (commentId: string, emoji: string) => void; } - let { comment, wsSlug, username = '', items, currentUserId = '', canEdit = true, frozen = false, attachmentResolver, isAdmin = false, hostToken = '', onDelete, onReply, onEdit, onReaction, onRemoveReaction }: Props = $props(); + let { comment, onUpdate = false, wsSlug, username = '', items, currentUserId = '', canEdit = true, frozen = false, attachmentResolver, isAdmin = false, hostToken = '', onDelete, onReply, onEdit, onReaction, onRemoveReaction }: Props = $props(); let showReplyForm = $state(false); let submittingReply = $state(false); @@ -247,7 +253,7 @@ {/if} - {#if comment.activity_id} + {#if onUpdate}
commented on update
{/if} diff --git a/web/src/lib/components/timeline/TimelineEntryList.svelte b/web/src/lib/components/timeline/TimelineEntryList.svelte index ff27db71b..7e3fb5220 100644 --- a/web/src/lib/components/timeline/TimelineEntryList.svelte +++ b/web/src/lib/components/timeline/TimelineEntryList.svelte @@ -135,6 +135,7 @@ {#if entry.kind === 'comment' && entry.comment} = {}): Comment { + return { + id: 'c-1', + item_id: 'item-1', + workspace_id: 'ws-1', + author: 'Dave', + body: 'hello', + created_by: 'user', + source: 'cli', + activity_id: 'a-1', // every comment has one + created_at: new Date('2026-10-06T12:00:00Z').toISOString(), + updated_at: new Date('2026-10-06T12:00:00Z').toISOString(), + ...overrides + }; +} + +const noop = () => {}; +const handlers = { onDelete: noop, onReply: noop, onEdit: noop, onReaction: noop, onRemoveReaction: noop }; + +function entry(onUpdate: boolean | undefined): TimelineEntry { + return { + id: 'c-1', + kind: 'comment', + created_at: '2026-10-06T12:00:00Z', + actor: 'user', + source: 'cli', + comment: comment(), + ...(onUpdate === undefined ? {} : { comment_on_update: onUpdate }) + } as TimelineEntry; +} + +describe('"commented on update" label (BUG-3437)', () => { + it('a standalone comment with an activity_id shows no label', () => { + const { queryByText } = render(TimelineCommentCard, { comment: comment(), wsSlug: 'ws', items: [], ...handlers }); + expect(queryByText('commented on update')).toBeNull(); + }); + + it('the card shows it when told the comment came with an update', () => { + const { getByText } = render(TimelineCommentCard, { comment: comment(), onUpdate: true, wsSlug: 'ws', items: [], ...handlers }); + expect(getByText('commented on update')).toBeTruthy(); + }); + + it('the entry list passes comment_on_update through, and only true counts', () => { + for (const [flag, shown] of [ + [true, true], + [false, false], + [undefined, false] + ] as const) { + const { queryByText, unmount } = render(TimelineEntryList, { entries: [entry(flag)], wsSlug: 'ws', items: [], ...handlers }); + expect(queryByText('commented on update') !== null, `comment_on_update=${flag}`).toBe(shown); + unmount(); + } + }); +}); diff --git a/web/src/lib/components/timeline/timelineCommentOnUpdateLive.svelte.test.ts b/web/src/lib/components/timeline/timelineCommentOnUpdateLive.svelte.test.ts new file mode 100644 index 000000000..ecf709e45 --- /dev/null +++ b/web/src/lib/components/timeline/timelineCommentOnUpdateLive.svelte.test.ts @@ -0,0 +1,189 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { flushSync, mount, unmount, tick } from 'svelte'; +import type { Comment, TimelineEntry, TimelineResponse } from '$lib/types'; + +/** + * BUG-3437, the live half. "commented on update" comes from the timeline + * entry's comment_on_update, which only the SERVER sets. A comment that + * arrives while the timeline is open (an SSE comment_created, or an update + * with --comment from the CLI or MCP) reaches the view through the SSE + * refresh, which re-reads the timeline endpoint, so it carries the flag like + * a reload does. This pins that: if a future change built the entry + * client-side, the label would silently disappear until reload. + * + * Harness copied from timelineRefreshDeletion.svelte.test.ts (module mocks + * are per-file). + */ + +type ListParams = { limit?: number; before?: string; before_id?: string; kinds?: readonly string[] } | undefined; + +const pages: TimelineResponse[] = []; +const timelineListMock = vi.fn(async (_ws: string, _slug: string, _params: ListParams) => { + return pages.shift() ?? { entries: [], has_more: false }; +}); + +vi.mock('$lib/api/client', () => ({ + api: { + timeline: { + list: (ws: string, slug: string, params: ListParams) => timelineListMock(ws, slug, params), + }, + comments: { + create: vi.fn(), + update: vi.fn(), + delete: vi.fn(), + addReaction: vi.fn(), + removeReaction: vi.fn(), + }, + attachments: { + downloadUrl: (ws: string, id: string) => `/api/v1/workspaces/${ws}/attachments/${id}`, + }, + }, +})); + +// FANS OUT rather than only recording: a mock that returns a disposer leaves +// the component subscribed to nothing and every refresh assertion passes +// vacuously (BUG-2509's lesson). +let itemEventCb: ((event: { type: string }) => void) | undefined; +vi.mock('$lib/services/sse.svelte', () => ({ + sseService: { + onItemEvent: (cb: (event: { type: string }) => void) => { + itemEventCb = cb; + return () => { + itemEventCb = undefined; + }; + }, + }, +})); + +vi.mock('$lib/stores/auth.svelte', () => ({ + authStore: { + userId: 'user-1', + user: { id: 'user-1', role: 'member' }, + // ItemTimeline fences every load and handler on identity (BUG-3105); + // identity never changes in this suite. + identityFence: () => () => true, + }, +})); + +vi.mock('$lib/stores/workspace.svelte', () => ({ + workspaceStore: { canEditItem: () => false }, +})); + +vi.mock('$lib/components/CommentEditor.svelte', async () => ({ + default: (await import('./fixtures/InertCommentEditor.svelte')).default, +})); + +const { default: ItemTimeline } = await import('./ItemTimeline.svelte'); + +function entry(id: string, createdAt: string, body: string, onUpdate?: boolean): TimelineEntry { + const c: Comment = { + id, + item_id: 'item-a', + workspace_id: 'ws-1', + author: 'alice', + body, + created_by: 'alice', + source: 'web', + created_at: createdAt, + updated_at: createdAt, + }; + return { + id, + kind: 'comment', + created_at: createdAt, + actor: 'alice', + source: 'cli', + comment: { ...c, activity_id: `act-${id}` }, // every comment has one + ...(onUpdate ? { comment_on_update: true } : {}) + }; +} + +let host: HTMLElement; +let app: Record | null = null; + +const props = $state({ + wsSlug: 'ws', + username: 'alice', + itemSlug: 'TASK-1', + currentContent: '', + itemId: 'item-a', + collectionId: 'coll-1', + hostToken: 'host-1', + resourceGen: 0, + visibleKinds: undefined as Array<'comment' | 'activity' | 'version'> | undefined, + mutationsEnabled: false, +}); + +async function settle() { + for (let i = 0; i < 10; i++) { + await tick(); + flushSync(); + } +} + +/** + * Fire a relevant SSE event and let the debounced refresh run. The component + * debounces these by 500ms, so the wait has to clear it — and the helper + * ASSERTS the refresh actually happened, because every expectation below is + * about what the refresh did and would pass vacuously if it never ran. + */ +async function fireRefresh() { + const before = timelineListMock.mock.calls.length; + itemEventCb?.({ type: 'comment_created' }); + await new Promise((r) => setTimeout(r, 700)); + await settle(); + if (timelineListMock.mock.calls.length === before) { + throw new Error('the SSE refresh never fired — every assertion after this would be vacuous'); + } +} + +function shownBodies(): string[] { + return Array.from(host.querySelectorAll('.comment-card')).map((el) => + (el.textContent ?? '').trim() + ); +} + +function shows(label: string): boolean { + return shownBodies().some((b) => b.includes(label)); +} + +beforeEach(() => { + host = document.createElement('div'); + document.body.appendChild(host); + pages.length = 0; + timelineListMock.mockClear(); +}); + +afterEach(() => { + if (app) unmount(app); + app = null; + host.remove(); +}); + +function labelled(body: string): boolean { + const card = Array.from(host.querySelectorAll('.comment-card')).find((el) => (el.textContent ?? '').includes(body)); + if (!card) throw new Error(`no card for "${body}"`); + return (card.textContent ?? '').includes('commented on update'); +} + +describe('"commented on update" on a comment that arrives live (BUG-3437)', () => { + it('an SSE refresh brings the label for an update\'s comment, and not for a plain one', async () => { + pages.push({ entries: [entry('e-plain', '2026-10-06T12:00:01Z', 'a plain comment')], has_more: false }); + pages.push({ + entries: [ + entry('e-upd', '2026-10-06T12:00:09Z', 'why I renamed it', true), + entry('e-plain', '2026-10-06T12:00:01Z', 'a plain comment') + ], + has_more: false + }); + + app = mount(ItemTimeline, { target: host, props }) as Record; + await settle(); + expect(labelled('a plain comment')).toBe(false); + + await fireRefresh(); + + expect(labelled('why I renamed it'), 'the live update comment lost its label').toBe(true); + expect(labelled('a plain comment')).toBe(false); + }); +}); diff --git a/web/src/lib/types/index.ts b/web/src/lib/types/index.ts index 497170a73..540d71378 100644 --- a/web/src/lib/types/index.ts +++ b/web/src/lib/types/index.ts @@ -1730,6 +1730,8 @@ export interface TimelineEntry { * second copy here (TASK-2760). */ agent_name?: string; + /** A comment entry whose comment an item update carried (BUG-3437); absent for a standalone comment. */ + comment_on_update?: boolean; source: string; comment?: Comment; activity?: Activity;