From 8ba64de90ba8f8f06f97c9f7498ec690bfd5b878 Mon Sep 17 00:00:00 2001 From: Stefan Ernst <24554551+stefan-ernst@users.noreply.github.com> Date: Wed, 21 Oct 2026 12:30:18 +0200 Subject: [PATCH] perf(api): cache and opt in to comment reaction reads The item view re-reads grouped reactions on every comment feed page, and the SSE reconcile re-fetches every loaded page, so a comment table read rode along on each one. Serve grouped reactions from a short-TTL process cache invalidated by reaction writes and comment deletes, and make hydration opt-in so feeds that never render chips skip the read entirely. - comment_reactions reads now consult a per-comment cache (5s TTL, bounded entries). AddReaction, RemoveReaction, and comment deletion invalidate the entry; the TTL only bounds staleness across processes. - GET /items/{item_id}/comments, GET /comments/{comment_id}, and PATCH /comments/{comment_id} attach reactions only when include_reactions is true; POST /comments/batch takes the same opt-in field. The comment UI opts in; other consumers no longer pay for it. --- api/openapi-v2.json | 36 +++++- frontend/src/lib/api/misc.js | 2 + .../src/lib/features/items/Comments.svelte | 7 +- internal/restapi/v2/comments.go | 35 +++-- internal/restapi/v2/contract-metadata.json | 32 ++++- internal/restapi/v2/router.go | 7 +- internal/services/comment_reactions.go | 121 ++++++++++++++++-- internal/services/comment_service.go | 4 + 8 files changed, 220 insertions(+), 24 deletions(-) diff --git a/api/openapi-v2.json b/api/openapi-v2.json index 6d4b5ddd1..cdcb3ee49 100644 --- a/api/openapi-v2.json +++ b/api/openapi-v2.json @@ -65625,6 +65625,10 @@ "PostCommentsBatchPayload": { "additionalProperties": false, "properties": { + "include_reactions": { + "description": "The include reactions value.", + "type": "boolean" + }, "item_ids": { "description": "The item ids value.", "items": { @@ -103802,6 +103806,16 @@ "minimum": 1, "type": "integer" } + }, + { + "description": "Attach grouped emoji reactions to the returned comment. Values true and 1 enable; omitted or other values leave reactions out.", + "in": "query", + "name": "include_reactions", + "required": false, + "schema": { + "default": false, + "type": "boolean" + } } ], "responses": { @@ -103857,6 +103871,16 @@ "minimum": 1, "type": "integer" } + }, + { + "description": "Attach grouped emoji reactions to the returned comment. Values true and 1 enable; omitted or other values leave reactions out.", + "in": "query", + "name": "include_reactions", + "required": false, + "schema": { + "default": false, + "type": "boolean" + } } ], "requestBody": { @@ -108121,7 +108145,7 @@ }, "/items/{item_id}/comments": { "get": { - "description": "Returns an authorization-filtered comment feed with at most 200 entries. Opaque next_cursor and refresh_cursor values preserve the exclusive timestamp-and-ID boundary without exposing its representation; partial comment pages are never returned on failure.", + "description": "Returns an authorization-filtered comment feed with at most 200 entries. Opaque next_cursor and refresh_cursor values preserve the exclusive timestamp-and-ID boundary without exposing its representation; partial comment pages are never returned on failure. Grouped reactions are attached only when include_reactions is true.", "operationId": "getItemsItemIdComments", "parameters": [ { @@ -108155,6 +108179,16 @@ "minLength": 1, "type": "string" } + }, + { + "description": "Attach grouped emoji reactions to each comment. Values true and 1 enable; omitted or other values leave reactions out.", + "in": "query", + "name": "include_reactions", + "required": false, + "schema": { + "default": false, + "type": "boolean" + } } ], "responses": { diff --git a/frontend/src/lib/api/misc.js b/frontend/src/lib/api/misc.js index c81298d47..2b2d71731 100644 --- a/frontend/src/lib/api/misc.js +++ b/frontend/src/lib/api/misc.js @@ -65,6 +65,8 @@ export const getComments = (itemId, params = {}) => { const searchParams = new URLSearchParams(); if (params.limit) searchParams.set('page_size', params.limit); if (params.cursor) searchParams.set('cursor', params.cursor); + // Reactions are opt-in so feeds that do not render chips skip the extra read. + if (params.include_reactions) searchParams.set('include_reactions', 'true'); const query = searchParams.toString(); return fetchV2Data(`/items/${itemId}/comments${query ? `?${query}` : ''}`); }; diff --git a/frontend/src/lib/features/items/Comments.svelte b/frontend/src/lib/features/items/Comments.svelte index 63f28698f..6eddc935a 100644 --- a/frontend/src/lib/features/items/Comments.svelte +++ b/frontend/src/lib/features/items/Comments.svelte @@ -161,7 +161,7 @@ const version = ++reconciliationVersion; let response; try { - response = await loadAttributedComments(itemId, { limit: COMMENT_PAGE_SIZE }); + response = await loadAttributedComments(itemId, { limit: COMMENT_PAGE_SIZE, include_reactions: true }); } catch (err) { if (version !== reconciliationVersion) return; // Navigation can unload the document while this request is in flight. @@ -203,6 +203,7 @@ try { response = await loadAttributedComments(itemId, { limit: COMMENT_PAGE_SIZE, + include_reactions: true, ...(refreshCursor ? { cursor: refreshCursor } : {}) }); } catch (err) { @@ -256,6 +257,7 @@ const isFirstPage = firstResponse === null; const response = await loadAttributedComments(itemId, { limit: Math.min(100, targetCount - fresh.length), + include_reactions: true, ...(cursor ? { cursor } : {}) }); if (version !== reconciliationVersion) return; @@ -313,7 +315,8 @@ try { const response = await loadAttributedComments(itemId, { limit: COMMENT_PAGE_SIZE, - cursor: olderCursor + cursor: olderCursor, + include_reactions: true }); comments = mergeCommentRows(comments, response?.comments || []); hasMore = Boolean(response?.has_more); diff --git a/internal/restapi/v2/comments.go b/internal/restapi/v2/comments.go index fb6deb7f0..8d3f57fc6 100644 --- a/internal/restapi/v2/comments.go +++ b/internal/restapi/v2/comments.go @@ -46,8 +46,9 @@ type commentCreateRequest struct { const maxCommentBatchItems = 500 type commentBatchRequest struct { - ItemIDs []int `json:"item_ids"` - PageSize int `json:"page_size"` + ItemIDs []int `json:"item_ids"` + PageSize int `json:"page_size"` + IncludeReactions bool `json:"include_reactions,omitempty"` } type commentBatchEntry struct { @@ -115,8 +116,10 @@ func commentsBatch(deps Deps) jsonOperation[commentBatchRequest, []commentBatchE if err != nil { return nil, internalError(err) } - if err := attachBatchReactions(deps, pages); err != nil { - return nil, internalError(err) + if input.IncludeReactions { + if err := attachBatchReactions(deps, pages); err != nil { + return nil, internalError(err) + } } entries := make([]commentBatchEntry, 0, len(visible)) for _, itemID := range visible { @@ -199,8 +202,12 @@ func listComments(deps Deps) readOperation[commentFeedDTO] { if err := renderCommentHTML(page.Comments); err != nil { return commentFeedDTO{}, internalError(err) } - if err := deps.Comments.AttachReactions(page.Comments); err != nil { - return commentFeedDTO{}, internalError(err) + if includeReactions, err := optionalBoolQuery(r, "include_reactions"); err != nil { + return commentFeedDTO{}, err + } else if includeReactions { + if err := deps.Comments.AttachReactions(page.Comments); err != nil { + return commentFeedDTO{}, internalError(err) + } } var total *int if options.Before == nil && options.Since == nil { @@ -257,8 +264,12 @@ func getComment(deps Deps) readOperation[models.Comment] { return models.Comment{}, err } body := []models.Comment{comment.Comment} - if err := deps.Comments.AttachReactions(body); err != nil { - return models.Comment{}, internalError(err) + if includeReactions, err := optionalBoolQuery(r, "include_reactions"); err != nil { + return models.Comment{}, err + } else if includeReactions { + if err := deps.Comments.AttachReactions(body); err != nil { + return models.Comment{}, internalError(err) + } } body[0].ContentHTML, err = markdown.Render(body[0].Content) return body[0], commentError(err) @@ -286,8 +297,12 @@ func updateComment(deps Deps) jsonOperation[commentPatchRequest, models.Comment] return models.Comment{}, commentError(err) } body := []models.Comment{*updated} - if err := deps.Comments.AttachReactions(body); err != nil { - return models.Comment{}, internalError(err) + if includeReactions, err := optionalBoolQuery(r, "include_reactions"); err != nil { + return models.Comment{}, err + } else if includeReactions { + if err := deps.Comments.AttachReactions(body); err != nil { + return models.Comment{}, internalError(err) + } } body[0].ContentHTML, err = markdown.Render(body[0].Content) return body[0], commentError(err) diff --git a/internal/restapi/v2/contract-metadata.json b/internal/restapi/v2/contract-metadata.json index 142adf424..97b6bf158 100644 --- a/internal/restapi/v2/contract-metadata.json +++ b/internal/restapi/v2/contract-metadata.json @@ -3788,6 +3788,16 @@ "minimum": 1, "type": "integer" } + }, + { + "Name": "include_reactions", + "In": "query", + "Description": "Attach grouped emoji reactions to the returned comment. Values true and 1 enable; omitted or other values leave reactions out.", + "Required": false, + "Schema": { + "default": false, + "type": "boolean" + } } ], "responses": { @@ -3814,6 +3824,16 @@ "minimum": 1, "type": "integer" } + }, + { + "Name": "include_reactions", + "In": "query", + "Description": "Attach grouped emoji reactions to the returned comment. Values true and 1 enable; omitted or other values leave reactions out.", + "Required": false, + "Schema": { + "default": false, + "type": "boolean" + } } ], "responses": { @@ -5967,7 +5987,7 @@ }, "/items/{item_id}/comments": { "get": { - "description": "Returns an authorization-filtered comment feed with at most 200 entries. Opaque next_cursor and refresh_cursor values preserve the exclusive timestamp-and-ID boundary without exposing its representation; partial comment pages are never returned on failure.", + "description": "Returns an authorization-filtered comment feed with at most 200 entries. Opaque next_cursor and refresh_cursor values preserve the exclusive timestamp-and-ID boundary without exposing its representation; partial comment pages are never returned on failure. Grouped reactions are attached only when include_reactions is true.", "parameters": [ { "Name": "item_id", @@ -6000,6 +6020,16 @@ "minLength": 1, "type": "string" } + }, + { + "Name": "include_reactions", + "In": "query", + "Description": "Attach grouped emoji reactions to each comment. Values true and 1 enable; omitted or other values leave reactions out.", + "Required": false, + "Schema": { + "default": false, + "type": "boolean" + } } ], "responses": { diff --git a/internal/restapi/v2/router.go b/internal/restapi/v2/router.go index d3df68464..fdc5ebe67 100644 --- a/internal/restapi/v2/router.go +++ b/internal/restapi/v2/router.go @@ -1023,6 +1023,11 @@ func applyParameterCorrections(route *Route) { defaultSize = 200 } upsertParameter(route, integerQuery("page_size", "Maximum number of resources to return.", 200, defaultSize)) + if route.Path == "/items/{item_id}/comments" { + upsertParameter(route, booleanQuery("include_reactions", "Attach grouped emoji reactions to each comment. Values true and 1 enable; omitted or other values leave reactions out.", false)) + } + case "GET /comments/{comment_id}", "PATCH /comments/{comment_id}": + upsertParameter(route, booleanQuery("include_reactions", "Attach grouped emoji reactions to the returned comment. Values true and 1 enable; omitted or other values leave reactions out.", false)) case "POST /items/batch", "POST /assets/summaries", "POST /milestones/test-statistics", "POST /iterations/progress": removeQueryParameter(route, "ids") } @@ -1203,7 +1208,7 @@ func applyParameterCorrections(route *Route) { case "GET /links/batch": route.Description = "Returns one-hop links for at most 100 explicit item IDs, or for one paged query-language selection. Explicit IDs retain first-occurrence order. Each item is capped independently and exposes an operation-bound next_cursor when more links exist; inaccessible links are omitted." case "GET /items/{item_id}/comments": - route.Description = "Returns an authorization-filtered comment feed with at most 200 entries. Opaque next_cursor and refresh_cursor values preserve the exclusive timestamp-and-ID boundary without exposing its representation; partial comment pages are never returned on failure." + route.Description = "Returns an authorization-filtered comment feed with at most 200 entries. Opaque next_cursor and refresh_cursor values preserve the exclusive timestamp-and-ID boundary without exposing its representation; partial comment pages are never returned on failure. Grouped reactions are attached only when include_reactions is true." case "GET /agent-runs/{run_id}/events": route.Description = "Returns up to 200 visible run events after an operation-bound opaque cursor. Events use stable ascending ID order, next_cursor resumes exclusively after the final returned event, and partial pages are never returned on failure." case "GET /workspaces/{workspace_id}/agent-runs", "GET /items/{item_id}/agent-runs": diff --git a/internal/services/comment_reactions.go b/internal/services/comment_reactions.go index 27dcf4d8e..220bf8d0d 100644 --- a/internal/services/comment_reactions.go +++ b/internal/services/comment_reactions.go @@ -5,6 +5,8 @@ import ( "errors" "fmt" "slices" + "sync" + "time" "windshift/internal/models" "windshift/internal/repository" @@ -22,6 +24,81 @@ var ErrInvalidCommentReaction = errors.New("reaction is not supported") // MaxCommentFeedLimit rows per item, but batches span many items. const maxReactionCommentIDs = 500 +// reactionCacheTTL bounds how long grouped reactions are served from process +// memory without a database read. Writes invalidate the affected comment in +// this process; the TTL only bounds staleness for other processes in a +// multi-instance deployment. +const reactionCacheTTL = 5 * time.Second + +// reactionCacheMaxEntries caps memory. Entries expire lazily, so a process can +// hold at most this many between prunes. +const reactionCacheMaxEntries = 20000 + +type reactionCacheEntry struct { + groups []models.CommentReaction + expiresAt time.Time +} + +// commentReactionCache memoizes grouped reactions per comment. The zero value +// is ready to use. Callers must treat the returned groups as read-only. +type commentReactionCache struct { + mu sync.Mutex + entries map[int]reactionCacheEntry + // now is the clock used for expiry; tests override it. Nil means time.Now. + now func() time.Time +} + +func (c *commentReactionCache) clock() time.Time { + if c.now != nil { + return c.now() + } + return time.Now() +} + +func (c *commentReactionCache) get(commentID int) ([]models.CommentReaction, bool) { + c.mu.Lock() + defer c.mu.Unlock() + entry, ok := c.entries[commentID] + if !ok { + return nil, false + } + if c.clock().After(entry.expiresAt) { + delete(c.entries, commentID) + return nil, false + } + return entry.groups, true +} + +func (c *commentReactionCache) set(commentID int, groups []models.CommentReaction) { + c.mu.Lock() + defer c.mu.Unlock() + if c.entries == nil { + c.entries = make(map[int]reactionCacheEntry) + } + if len(c.entries) >= reactionCacheMaxEntries { + c.pruneExpiredLocked() + } + if len(c.entries) >= reactionCacheMaxEntries { + clear(c.entries) + } + c.entries[commentID] = reactionCacheEntry{groups: groups, expiresAt: c.clock().Add(reactionCacheTTL)} +} + +func (c *commentReactionCache) invalidate(commentID int) { + c.mu.Lock() + defer c.mu.Unlock() + delete(c.entries, commentID) +} + +func (c *commentReactionCache) pruneExpiredLocked() { + now := c.clock() + for id, entry := range c.entries { + if now.After(entry.expiresAt) { + delete(c.entries, id) + } + } +} + // ValidCommentReaction reports whether key is a supported reaction. func ValidCommentReaction(key string) bool { return slices.Contains(CommentReactionKeys, key) @@ -42,6 +119,7 @@ func (s *CommentService) AddReaction(commentID, userID int, reaction string) ([] ON CONFLICT (comment_id, user_id, reaction) DO NOTHING`, commentID, userID, reaction); err != nil { return nil, fmt.Errorf("add reaction to comment %d: %w", commentID, err) } + s.reactionCache.invalidate(commentID) PublishItemChange(itemID) return s.ReactionsForComment(commentID) } @@ -60,6 +138,7 @@ func (s *CommentService) RemoveReaction(commentID, userID int, reaction string) commentID, userID, reaction); err != nil { return fmt.Errorf("remove reaction from comment %d: %w", commentID, err) } + s.reactionCache.invalidate(commentID) PublishItemChange(itemID) return nil } @@ -109,9 +188,22 @@ func (s *CommentService) reactionCommentItem(commentID int) (int, error) { } func (s *CommentService) reactionsByComment(ids []int) (map[int][]models.CommentReaction, error) { - result := make(map[int][]models.CommentReaction) - for start := 0; start < len(ids); start += maxReactionCommentIDs { - chunk := ids[start:min(start+maxReactionCommentIDs, len(ids))] + result := make(map[int][]models.CommentReaction, len(ids)) + missing := make([]int, 0, len(ids)) + seen := make(map[int]struct{}, len(ids)) + for _, id := range ids { + if _, ok := seen[id]; ok { + continue + } + seen[id] = struct{}{} + if groups, ok := s.reactionCache.get(id); ok { + result[id] = groups + continue + } + missing = append(missing, id) + } + for start := 0; start < len(missing); start += maxReactionCommentIDs { + chunk := missing[start:min(start+maxReactionCommentIDs, len(missing))] args := make([]any, len(chunk)) for i, id := range chunk { args[i] = id @@ -126,6 +218,7 @@ func (s *CommentService) reactionsByComment(ids []int) (map[int][]models.Comment if err != nil { return nil, fmt.Errorf("list comment reactions: %w", err) } + found := make(map[int][]models.CommentReaction, len(chunk)) for rows.Next() { var commentID, userID int var reaction, name string @@ -133,22 +226,32 @@ func (s *CommentService) reactionsByComment(ids []int) (map[int][]models.Comment _ = rows.Close() return nil, fmt.Errorf("scan comment reaction: %w", err) } - result[commentID] = appendReaction(result[commentID], reaction, userID, name) + found[commentID] = appendReaction(found[commentID], reaction, userID, name) } err = rows.Err() _ = rows.Close() if err != nil { return nil, fmt.Errorf("read comment reactions: %w", err) } - } - for id := range result { - slices.SortStableFunc(result[id], func(a, b models.CommentReaction) int { - return slices.Index(CommentReactionKeys, a.Reaction) - slices.Index(CommentReactionKeys, b.Reaction) - }) + for _, id := range chunk { + groups := found[id] + if groups == nil { + groups = []models.CommentReaction{} + } + sortReactionGroups(groups) + s.reactionCache.set(id, groups) + result[id] = groups + } } return result, nil } +func sortReactionGroups(groups []models.CommentReaction) { + slices.SortStableFunc(groups, func(a, b models.CommentReaction) int { + return slices.Index(CommentReactionKeys, a.Reaction) - slices.Index(CommentReactionKeys, b.Reaction) + }) +} + func appendReaction(groups []models.CommentReaction, reaction string, userID int, name string) []models.CommentReaction { for i := range groups { if groups[i].Reaction == reaction { diff --git a/internal/services/comment_service.go b/internal/services/comment_service.go index 4c11dff75..3f8fad369 100644 --- a/internal/services/comment_service.go +++ b/internal/services/comment_service.go @@ -112,6 +112,7 @@ type CommentService struct { emailReplyService EmailReplyHandler agentMentionTrigger AgentMentionTrigger issueSync CommentIssueSync + reactionCache commentReactionCache } type CaptureComment struct { @@ -1252,6 +1253,9 @@ func (s *CommentService) Delete(commentID int) error { return fmt.Errorf("failed to commit comment deletion: %w", err) } + // The comment and its reactions are gone; drop any memoized groups. + s.reactionCache.invalidate(commentID) + // Live-update publish (WI-483): the delete committed. PublishItemChange(itemID)