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) <noreply@anthropic.com>
This commit is contained in:
Naiyuan Qing 2026-03-29 00:21:46 +08:00
parent 5d2e62ccde
commit b2ee151306
11 changed files with 267 additions and 122 deletions

4
.gitignore vendored
View file

@ -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

View file

@ -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();
});

View file

@ -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",

View file

@ -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<string, TimelineEntry[]>;
currentUserId?: string;
onReply: (parentId: string, content: string) => Promise<void>;
@ -93,7 +78,7 @@ function CommentRow({
};
return (
<div className={`group/comment py-3${isTemp ? " opacity-60" : ""}`}>
<div className={`py-3${isTemp ? " opacity-60" : ""}`}>
<div className="flex items-center gap-2.5">
<ActorAvatar actorType={entry.actor_type} actorId={entry.actor_id} size={24} />
<span className="text-sm font-medium">
@ -112,23 +97,23 @@ function CommentRow({
</TooltipContent>
</Tooltip>
{!isTemp && (isOwn) && (
<div className="ml-auto opacity-0 group-hover/comment:opacity-100 transition-opacity">
<DropdownMenu>
<DropdownMenuTrigger
className="inline-flex h-6 w-6 items-center justify-center rounded-md text-muted-foreground hover:text-foreground hover:bg-accent/50 data-[popup-open]:opacity-100 data-[popup-open]:bg-accent/50 transition-colors"
>
<MoreHorizontal className="h-4 w-4" />
</DropdownMenuTrigger>
<DropdownMenuContent align="end">
<DropdownMenuItem onSelect={startEdit}>Edit</DropdownMenuItem>
<DropdownMenuSeparator />
<DropdownMenuItem onSelect={() => onDelete(entry.id)} variant="destructive">
Delete
</DropdownMenuItem>
</DropdownMenuContent>
</DropdownMenu>
</div>
{!isTemp && isOwn && (
<DropdownMenu>
<DropdownMenuTrigger
render={
<Button variant="ghost" size="icon-xs" className="ml-auto text-muted-foreground">
<MoreHorizontal className="h-4 w-4" />
</Button>
}
/>
<DropdownMenuContent align="end">
<DropdownMenuItem onClick={startEdit}>Edit</DropdownMenuItem>
<DropdownMenuSeparator />
<DropdownMenuItem onClick={() => onDelete(entry.id)} variant="destructive">
Delete
</DropdownMenuItem>
</DropdownMenuContent>
</DropdownMenu>
)}
</div>
@ -165,7 +150,6 @@ function CommentRow({
function CommentCard({
entry,
replies,
allReplies,
currentUserId,
onReply,

View file

@ -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<string, string>;
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<string>([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<string>([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) {
<div className="flex items-center justify-between">
<div className="flex items-center gap-3">
<h2 className="text-base font-semibold">Activity</h2>
<div className="flex gap-1">
</div>
</div>
<div className="flex items-center gap-2">
<button
@ -811,7 +844,6 @@ export function IssueDetail({ issueId, onDelete }: IssueDetailProps) {
{/* Timeline entries */}
<div className="mt-4 space-y-2">
{(() => {
// Separate top-level entries from replies
const topLevel = timeline.filter((e) => e.type === "activity" || !e.parent_id);
const repliesByParent = new Map<string, TimelineEntry[]>();
for (const e of timeline) {
@ -822,39 +854,83 @@ export function IssueDetail({ issueId, onDelete }: IssueDetailProps) {
}
}
return topLevel.map((entry) => {
// Group consecutive activities together so the connector line works
const groups: { type: "activities" | "comment"; entries: TimelineEntry[] }[] = [];
for (const entry of topLevel) {
if (entry.type === "activity") {
const last = groups[groups.length - 1];
if (last?.type === "activities") {
last.entries.push(entry);
} else {
groups.push({ type: "activities", entries: [entry] });
}
} else {
groups.push({ type: "comment", entries: [entry] });
}
}
return groups.map((group) => {
if (group.type === "comment") {
const entry = group.entries[0]!;
return (
<div key={entry.id} className="flex items-center gap-2.5 py-1.5 text-sm text-muted-foreground">
<ActorAvatar actorType={entry.actor_type} actorId={entry.actor_id} size={28} />
<span className="font-medium">{getActorName(entry.actor_type, entry.actor_id)}</span>
<span>{formatActivity(entry)}</span>
<Tooltip>
<TooltipTrigger
render={
<span className="ml-auto text-xs cursor-default">
{timeAgo(entry.created_at)}
</span>
}
/>
<TooltipContent side="top">
{new Date(entry.created_at).toLocaleString()}
</TooltipContent>
</Tooltip>
</div>
<CommentCard
key={entry.id}
entry={entry}
allReplies={repliesByParent}
currentUserId={user?.id}
onReply={handleSubmitReply}
onEdit={handleEditComment}
onDelete={handleDeleteComment}
/>
);
}
return (
<CommentCard
key={entry.id}
entry={entry}
replies={repliesByParent.get(entry.id) ?? []}
allReplies={repliesByParent}
currentUserId={user?.id}
onReply={handleSubmitReply}
onEdit={handleEditComment}
onDelete={handleDeleteComment}
/>
<div key={group.entries[0]!.id} className="px-4">
{group.entries.map((entry, idx) => {
const details = (entry.details ?? {}) as Record<string, string>;
const isStatusChange = entry.action === "status_changed";
const isPriorityChange = entry.action === "priority_changed";
const isDueDateChange = entry.action === "due_date_changed";
const isLast = idx === group.entries.length - 1;
let leadIcon: React.ReactNode;
if (isStatusChange && details.to) {
leadIcon = <StatusIcon status={details.to as IssueStatus} className="h-3.5 w-3.5 shrink-0" />;
} else if (isPriorityChange && details.to) {
leadIcon = <PriorityIcon priority={details.to as IssuePriority} className="h-3.5 w-3.5 shrink-0" />;
} else if (isDueDateChange) {
leadIcon = <Calendar className="h-3.5 w-3.5 shrink-0 text-muted-foreground" />;
} else {
leadIcon = <ActorAvatar actorType={entry.actor_type} actorId={entry.actor_id} size={14} />;
}
return (
<div key={entry.id} className="flex text-xs text-muted-foreground">
<div className="mr-2.5 flex w-3.5 shrink-0 flex-col items-center">
<div className="flex h-5 items-center">{leadIcon}</div>
{!isLast && <div className="w-px flex-1 bg-border" />}
</div>
<div className={`flex flex-1 items-baseline gap-1 ${!isLast ? "pb-3" : ""}`}>
<span className="font-medium">{getActorName(entry.actor_type, entry.actor_id)}</span>
<span>{formatActivity(entry, getActorName)}</span>
<Tooltip>
<TooltipTrigger
render={
<span className="ml-auto shrink-0 cursor-default">
{timeAgo(entry.created_at)}
</span>
}
/>
<TooltipContent side="top">
{new Date(entry.created_at).toLocaleString()}
</TooltipContent>
</Tooltip>
</div>
</div>
);
})}
</div>
);
});
})()}

View file

@ -49,35 +49,46 @@ function ReplyInput({
const avatarSize = size === "sm" ? 22 : 28;
return (
<div className="flex items-center gap-2.5">
<div className="flex items-start gap-2.5">
<ActorAvatar
actorType={avatarType}
actorId={avatarId}
size={avatarSize}
className="shrink-0"
className="mt-0.5 shrink-0"
/>
<div
className={`min-w-0 flex-1 overflow-y-auto ${
size === "sm" ? "max-h-32" : "max-h-48"
}`}
>
<RichTextEditor
ref={editorRef}
placeholder={placeholder}
onUpdate={(md) => setIsEmpty(!md.trim())}
onSubmit={handleSubmit}
debounceMs={100}
/>
<div className="min-w-0 flex-1">
<div
className={`overflow-y-auto text-sm ${
size === "sm" ? "max-h-32" : "max-h-48"
}`}
>
<RichTextEditor
ref={editorRef}
placeholder={placeholder}
onUpdate={(md) => setIsEmpty(!md.trim())}
onSubmit={handleSubmit}
debounceMs={100}
/>
</div>
<div
className={`grid transition-all duration-150 ${
isEmpty ? "grid-rows-[0fr] opacity-0" : "grid-rows-[1fr] opacity-100"
}`}
>
<div className="overflow-hidden">
<div className="flex items-center justify-end pt-1">
<Button
size="icon-xs"
disabled={isEmpty || submitting}
onClick={handleSubmit}
tabIndex={isEmpty ? -1 : 0}
>
<ArrowUp className="h-3.5 w-3.5" />
</Button>
</div>
</div>
</div>
</div>
<Button
size="icon-xs"
variant="ghost"
disabled={isEmpty || submitting}
onClick={handleSubmit}
className="shrink-0 text-muted-foreground hover:text-foreground"
>
<ArrowUp className="h-3.5 w-3.5" />
</Button>
</div>
);
}

10
apps/web/shared/utils.ts Normal file
View file

@ -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`;
}

View file

@ -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),
},

View file

@ -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{

View file

@ -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
`

View file

@ -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