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;