From b2ee1513065a97971390c313dfd7f4b845863748 Mon Sep 17 00:00:00 2001 From: Naiyuan Qing <145280634+NevilleQingNY@users.noreply.github.com> Date: Sun, 29 Mar 2026 00:21:46 +0800 Subject: [PATCH] fix(activity): address code review feedback and improve timeline UX - Extract shared timeAgo utility, remove duplicates from comment-card and issue-detail - Remove unused replies prop from CommentCard - Fix recursive delete to remove all descendant replies, not just direct children - Improve formatActivity with human-readable status/priority labels and actor names - Validate parent comment exists and belongs to same issue before creating reply - Add priority_changed activity recording in activity listeners - Fix activity SQL query to sort ASC (was DESC, then re-sorted in handler) - Fix reply-input layout alignment and test submit button selector - Minor: .gitignore additions, button dark mode aria-expanded fix Co-Authored-By: Claude Opus 4.6 (1M context) --- .gitignore | 4 + .../app/(dashboard)/issues/[id]/page.test.tsx | 11 +- apps/web/components/ui/button.tsx | 4 +- .../issues/components/comment-card.tsx | 54 ++--- .../issues/components/issue-detail.tsx | 186 ++++++++++++------ .../issues/components/reply-input.tsx | 57 +++--- apps/web/shared/utils.ts | 10 + server/cmd/server/activity_listeners.go | 54 ++++- server/internal/handler/comment.go | 5 + server/pkg/db/generated/activity.sql.go | 2 +- server/pkg/db/queries/activity.sql | 2 +- 11 files changed, 267 insertions(+), 122 deletions(-) create mode 100644 apps/web/shared/utils.ts diff --git a/.gitignore b/.gitignore index aed07640..b5e1c38a 100644 --- a/.gitignore +++ b/.gitignore @@ -5,6 +5,7 @@ dist .envrc # build outputs +.turbo .next out build @@ -36,6 +37,9 @@ apps/web/test-results/ # local settings .claude/ +# feature tracking +_features/ + # platform specific *.dmg *.app diff --git a/apps/web/app/(dashboard)/issues/[id]/page.test.tsx b/apps/web/app/(dashboard)/issues/[id]/page.test.tsx index 32cbbbac..84b53d68 100644 --- a/apps/web/app/(dashboard)/issues/[id]/page.test.tsx +++ b/apps/web/app/(dashboard)/issues/[id]/page.test.tsx @@ -281,11 +281,14 @@ describe("IssueDetailPage", () => { fireEvent.change(commentInput, { target: { value: "New test comment" } }); }); - // Wait for button to be enabled after commentEmpty state update - const allButtons = screen.getAllByRole("button"); - const submitBtn = allButtons.find( + // Find the submit button associated with the "Leave a comment..." input. + // Multiple ArrowUp buttons exist (one per ReplyInput), so we find the + // button within the same ReplyInput container as our textarea. + const allArrowUpBtns = screen.getAllByRole("button").filter( (btn) => btn.querySelector(".lucide-arrow-up") !== null, - )!; + ); + // The bottom "Leave a comment..." ReplyInput renders last, so its button is last + const submitBtn = allArrowUpBtns[allArrowUpBtns.length - 1]!; await waitFor(() => { expect(submitBtn).not.toBeDisabled(); }); diff --git a/apps/web/components/ui/button.tsx b/apps/web/components/ui/button.tsx index ded01b25..a91c4b5d 100644 --- a/apps/web/components/ui/button.tsx +++ b/apps/web/components/ui/button.tsx @@ -12,11 +12,11 @@ const buttonVariants = cva( variant: { default: "bg-primary text-primary-foreground [a]:hover:bg-primary/80", outline: - "border-border bg-background hover:bg-muted hover:text-foreground aria-expanded:bg-muted aria-expanded:text-foreground dark:border-input dark:bg-input/30 dark:hover:bg-input/50", + "border-border bg-background hover:bg-muted hover:text-foreground aria-expanded:bg-muted aria-expanded:text-foreground dark:border-input dark:bg-input/30 dark:hover:bg-input/50 dark:aria-expanded:bg-muted dark:aria-expanded:text-foreground", secondary: "bg-secondary text-secondary-foreground hover:bg-secondary/80 aria-expanded:bg-secondary aria-expanded:text-secondary-foreground", ghost: - "hover:bg-muted hover:text-foreground aria-expanded:bg-muted aria-expanded:text-foreground dark:hover:bg-muted/50", + "hover:bg-muted hover:text-foreground aria-expanded:bg-muted aria-expanded:text-foreground dark:hover:bg-muted/50 dark:aria-expanded:bg-muted dark:aria-expanded:text-foreground", destructive: "bg-destructive/10 text-destructive hover:bg-destructive/20 focus-visible:border-destructive/40 focus-visible:ring-destructive/20 dark:bg-destructive/20 dark:hover:bg-destructive/30 dark:focus-visible:ring-destructive/40", link: "text-primary underline-offset-4 hover:underline", diff --git a/apps/web/features/issues/components/comment-card.tsx b/apps/web/features/issues/components/comment-card.tsx index a03fc852..a8ea065a 100644 --- a/apps/web/features/issues/components/comment-card.tsx +++ b/apps/web/features/issues/components/comment-card.tsx @@ -16,31 +16,16 @@ import { Tooltip, TooltipTrigger, TooltipContent } from "@/components/ui/tooltip import { ActorAvatar } from "@/components/common/actor-avatar"; import { Markdown } from "@/components/markdown"; import { useActorName } from "@/features/workspace"; +import { timeAgo } from "@/shared/utils"; import { ReplyInput } from "./reply-input"; import type { TimelineEntry } from "@/shared/types"; -// --------------------------------------------------------------------------- -// Helpers -// --------------------------------------------------------------------------- - -function timeAgo(dateStr: string): string { - const diff = Date.now() - new Date(dateStr).getTime(); - const minutes = Math.floor(diff / 60000); - if (minutes < 1) return "just now"; - if (minutes < 60) return `${minutes}m ago`; - const hours = Math.floor(minutes / 60); - if (hours < 24) return `${hours}h ago`; - const days = Math.floor(hours / 24); - return `${days}d ago`; -} - // --------------------------------------------------------------------------- // Types // --------------------------------------------------------------------------- interface CommentCardProps { entry: TimelineEntry; - replies: TimelineEntry[]; allReplies: Map; currentUserId?: string; onReply: (parentId: string, content: string) => Promise; @@ -93,7 +78,7 @@ function CommentRow({ }; return ( -
+
@@ -112,23 +97,23 @@ function CommentRow({ - {!isTemp && (isOwn) && ( -
- - - - - - Edit - - onDelete(entry.id)} variant="destructive"> - Delete - - - -
+ {!isTemp && isOwn && ( + + + + + } + /> + + Edit + + onDelete(entry.id)} variant="destructive"> + Delete + + + )}
@@ -165,7 +150,6 @@ function CommentRow({ function CommentCard({ entry, - replies, allReplies, currentUserId, onReply, diff --git a/apps/web/features/issues/components/issue-detail.tsx b/apps/web/features/issues/components/issue-detail.tsx index f4bc29d0..bb2120cf 100644 --- a/apps/web/features/issues/components/issue-detail.tsx +++ b/apps/web/features/issues/components/issue-detail.tsx @@ -67,21 +67,7 @@ import { useWorkspaceStore, useActorName } from "@/features/workspace"; import { useWSEvent } from "@/features/realtime"; import { useIssueStore } from "@/features/issues"; import type { CommentCreatedPayload, CommentUpdatedPayload, CommentDeletedPayload, SubscriberAddedPayload, SubscriberRemovedPayload, ActivityCreatedPayload } from "@/shared/types"; - -// --------------------------------------------------------------------------- -// Helpers -// --------------------------------------------------------------------------- - -function timeAgo(dateStr: string): string { - const diff = Date.now() - new Date(dateStr).getTime(); - const minutes = Math.floor(diff / 60000); - if (minutes < 1) return "just now"; - if (minutes < 60) return `${minutes}m ago`; - const hours = Math.floor(minutes / 60); - if (hours < 24) return `${hours}h ago`; - const days = Math.floor(hours / 24); - return `${days}d ago`; -} +import { timeAgo } from "@/shared/utils"; function shortDate(date: string | null): string { if (!date) return "—"; @@ -91,15 +77,41 @@ function shortDate(date: string | null): string { }); } -function formatActivity(entry: TimelineEntry): string { +function statusLabel(status: string): string { + return STATUS_CONFIG[status as IssueStatus]?.label ?? status; +} + +function priorityLabel(priority: string): string { + return PRIORITY_CONFIG[priority as IssuePriority]?.label ?? priority; +} + +function formatActivity( + entry: TimelineEntry, + resolveActorName?: (type: string, id: string) => string, +): string { const details = (entry.details ?? {}) as Record; switch (entry.action) { case "created": return "created this issue"; case "status_changed": - return `changed status from ${details.from ?? "?"} to ${details.to ?? "?"}`; - case "assignee_changed": + return `changed status from ${statusLabel(details.from ?? "?")} to ${statusLabel(details.to ?? "?")}`; + case "priority_changed": + return `changed priority from ${priorityLabel(details.from ?? "?")} to ${priorityLabel(details.to ?? "?")}`; + case "assignee_changed": { + const isSelfAssign = details.to_type === entry.actor_type && details.to_id === entry.actor_id; + if (isSelfAssign) return "self-assigned this issue"; + const toName = details.to_id && details.to_type && resolveActorName + ? resolveActorName(details.to_type, details.to_id) + : null; + if (toName) return `assigned to ${toName}`; + if (details.from_id && !details.to_id) return "removed assignee"; return "changed assignee"; + } + case "due_date_changed": { + if (!details.to) return "removed due date"; + const formatted = new Date(details.to).toLocaleDateString("en-US", { month: "short", day: "numeric" }); + return `set due date to ${formatted}`; + } case "description_updated": return "updated the description"; case "task_completed": @@ -267,9 +279,21 @@ export function IssueDetail({ issueId, onDelete }: IssueDetailProps) { const handleDeleteComment = async (commentId: string) => { try { await api.deleteComment(commentId); - setTimeline((prev) => - prev.filter((e) => e.id !== commentId && e.parent_id !== commentId) - ); + setTimeline((prev) => { + const idsToRemove = new Set([commentId]); + // Recursively collect all descendant IDs + let added = true; + while (added) { + added = false; + for (const e of prev) { + if (e.parent_id && idsToRemove.has(e.parent_id) && !idsToRemove.has(e.id)) { + idsToRemove.add(e.id); + added = true; + } + } + } + return prev.filter((e) => !idsToRemove.has(e.id)); + }); } catch { toast.error("Failed to delete comment"); } @@ -359,9 +383,20 @@ export function IssueDetail({ issueId, onDelete }: IssueDetailProps) { useCallback((payload: unknown) => { const { comment_id, issue_id } = payload as CommentDeletedPayload; if (issue_id === id) { - setTimeline((prev) => - prev.filter((e) => e.id !== comment_id && e.parent_id !== comment_id) - ); + setTimeline((prev) => { + const idsToRemove = new Set([comment_id]); + let added = true; + while (added) { + added = false; + for (const e of prev) { + if (e.parent_id && idsToRemove.has(e.parent_id) && !idsToRemove.has(e.id)) { + idsToRemove.add(e.id); + added = true; + } + } + } + return prev.filter((e) => !idsToRemove.has(e.id)); + }); } }, [id]), ); @@ -727,8 +762,6 @@ export function IssueDetail({ issueId, onDelete }: IssueDetailProps) {

Activity

-
-
+
+
+
- ); } diff --git a/apps/web/shared/utils.ts b/apps/web/shared/utils.ts new file mode 100644 index 00000000..096f0f5d --- /dev/null +++ b/apps/web/shared/utils.ts @@ -0,0 +1,10 @@ +export function timeAgo(dateStr: string): string { + const diff = Date.now() - new Date(dateStr).getTime(); + const minutes = Math.floor(diff / 60000); + if (minutes < 1) return "just now"; + if (minutes < 60) return `${minutes}m ago`; + const hours = Math.floor(minutes / 60); + if (hours < 24) return `${hours}h ago`; + const days = Math.floor(hours / 24); + return `${days}d ago`; +} diff --git a/server/cmd/server/activity_listeners.go b/server/cmd/server/activity_listeners.go index 2a639a79..fbf5b359 100644 --- a/server/cmd/server/activity_listeners.go +++ b/server/cmd/server/activity_listeners.go @@ -59,6 +59,7 @@ func registerActivityListeners(bus *events.Bus, queries *db.Queries) { } statusChanged, _ := payload["status_changed"].(bool) + priorityChanged, _ := payload["priority_changed"].(bool) assigneeChanged, _ := payload["assignee_changed"].(bool) descriptionChanged, _ := payload["description_changed"].(bool) @@ -84,6 +85,28 @@ func registerActivityListeners(bus *events.Bus, queries *db.Queries) { } } + if priorityChanged { + prevPriority, _ := payload["prev_priority"].(string) + details, _ := json.Marshal(map[string]string{ + "from": prevPriority, + "to": issue.Priority, + }) + activity, err := queries.CreateActivity(ctx, db.CreateActivityParams{ + WorkspaceID: parseUUID(issue.WorkspaceID), + IssueID: parseUUID(issue.ID), + ActorType: util.StrToText(e.ActorType), + ActorID: parseUUID(e.ActorID), + Action: "priority_changed", + Details: details, + }) + if err != nil { + slog.Error("activity: failed to record priority change", + "issue_id", issue.ID, "error", err) + } else { + publishActivityEvent(bus, e, activity) + } + } + if assigneeChanged { prevAssigneeType, _ := payload["prev_assignee_type"].(*string) prevAssigneeID, _ := payload["prev_assignee_id"].(*string) @@ -119,6 +142,35 @@ func registerActivityListeners(bus *events.Bus, queries *db.Queries) { } } + if dueDateChanged, _ := payload["due_date_changed"].(bool); dueDateChanged { + prevDueDate := "" + if v, ok := payload["prev_due_date"].(*string); ok && v != nil { + prevDueDate = *v + } + newDueDate := "" + if issue.DueDate != nil { + newDueDate = *issue.DueDate + } + details, _ := json.Marshal(map[string]string{ + "from": prevDueDate, + "to": newDueDate, + }) + activity, err := queries.CreateActivity(ctx, db.CreateActivityParams{ + WorkspaceID: parseUUID(issue.WorkspaceID), + IssueID: parseUUID(issue.ID), + ActorType: util.StrToText(e.ActorType), + ActorID: parseUUID(e.ActorID), + Action: "due_date_changed", + Details: details, + }) + if err != nil { + slog.Error("activity: failed to record due date change", + "issue_id", issue.ID, "error", err) + } else { + publishActivityEvent(bus, e, activity) + } + } + if descriptionChanged { activity, err := queries.CreateActivity(ctx, db.CreateActivityParams{ WorkspaceID: parseUUID(issue.WorkspaceID), @@ -205,7 +257,7 @@ func publishActivityEvent(bus *events.Bus, original events.Event, activity db.Ac "id": util.UUIDToString(activity.ID), "actor_type": actorType, "actor_id": util.UUIDToString(activity.ActorID), - "action": &action, + "action": action, "details": json.RawMessage(activity.Details), "created_at": util.TimestampToString(activity.CreatedAt), }, diff --git a/server/internal/handler/comment.go b/server/internal/handler/comment.go index 99d45113..be6adb9f 100644 --- a/server/internal/handler/comment.go +++ b/server/internal/handler/comment.go @@ -94,6 +94,11 @@ func (h *Handler) CreateComment(w http.ResponseWriter, r *http.Request) { var parentID pgtype.UUID if req.ParentID != nil { parentID = parseUUID(*req.ParentID) + parent, err := h.Queries.GetComment(r.Context(), parentID) + if err != nil || uuidToString(parent.IssueID) != issueID { + writeError(w, http.StatusBadRequest, "invalid parent comment") + return + } } comment, err := h.Queries.CreateComment(r.Context(), db.CreateCommentParams{ diff --git a/server/pkg/db/generated/activity.sql.go b/server/pkg/db/generated/activity.sql.go index 87fffefb..af2394d9 100644 --- a/server/pkg/db/generated/activity.sql.go +++ b/server/pkg/db/generated/activity.sql.go @@ -53,7 +53,7 @@ func (q *Queries) CreateActivity(ctx context.Context, arg CreateActivityParams) const listActivities = `-- name: ListActivities :many SELECT id, workspace_id, issue_id, actor_type, actor_id, action, details, created_at FROM activity_log WHERE issue_id = $1 -ORDER BY created_at DESC +ORDER BY created_at ASC LIMIT $2 OFFSET $3 ` diff --git a/server/pkg/db/queries/activity.sql b/server/pkg/db/queries/activity.sql index 3e6dc39b..0e2dde33 100644 --- a/server/pkg/db/queries/activity.sql +++ b/server/pkg/db/queries/activity.sql @@ -1,7 +1,7 @@ -- name: ListActivities :many SELECT * FROM activity_log WHERE issue_id = $1 -ORDER BY created_at DESC +ORDER BY created_at ASC LIMIT $2 OFFSET $3; -- name: CreateActivity :one