From d861bff51e1597cd6902ff3d8ff601c3b1bf1451 Mon Sep 17 00:00:00 2001 From: Seongmin Lee Date: Mon, 8 Dec 2025 23:34:14 +0900 Subject: [PATCH] appview/notify: merge new comment events into one Signed-off-by: Seongmin Lee --- appview/issues/issues.go | 2 +- appview/notify/db/db.go | 232 ++++++++++++++--------------- appview/notify/logging_notifier.go | 13 +- appview/notify/merged_notifier.go | 16 +- appview/notify/notifier.go | 14 +- appview/notify/posthog/notifier.go | 22 +-- appview/pulls/pulls.go | 2 +- 7 files changed, 137 insertions(+), 164 deletions(-) diff --git a/appview/issues/issues.go b/appview/issues/issues.go index f2d2bb62..f6d8a200 100644 --- a/appview/issues/issues.go +++ b/appview/issues/issues.go @@ -489,7 +489,7 @@ func (rp *Issues) NewIssueComment(w http.ResponseWriter, r *http.Request) { // reset atUri to make rollback a no-op atUri = "" - rp.notifier.NewIssueComment(r.Context(), &comment, mentions) + rp.notifier.NewComment(r.Context(), &comment) ownerSlashRepo := reporesolver.GetBaseRepoPath(r, f) rp.pages.HxLocation(w, fmt.Sprintf("/%s/issues/%d#comment-%d", ownerSlashRepo, issue.IssueId, comment.Id)) diff --git a/appview/notify/db/db.go b/appview/notify/db/db.go index 993cc9d1..60aeb401 100644 --- a/appview/notify/db/db.go +++ b/appview/notify/db/db.go @@ -77,39 +77,112 @@ func (n *databaseNotifier) DeleteStar(ctx context.Context, star *models.Star) { // no-op } -func (n *databaseNotifier) NewIssue(ctx context.Context, issue *models.Issue, mentions []syntax.DID) { +func (n *databaseNotifier) NewComment(ctx context.Context, comment *models.Comment) { l := log.FromContext(ctx) - collaborators, err := db.GetCollaborators(n.db, orm.FilterEq("repo_at", issue.Repo.RepoAt())) + var ( + // built the recipients list: + // - the owner of the repo + // - | if the comment is a reply -> everybody on that thread + // | if the comment is a top level -> just the issue owner + // - remove mentioned users from the recipients list + recipients = sets.New[syntax.DID]() + entityType string + entityId string + repoId *int64 + issueId *int64 + pullId *int64 + ) + + subjectDid, err := comment.Subject.Authority().AsDID() if err != nil { - l.Error("failed to fetch collaborators", "err", err) + l.Error("expected did based at-uri for comment.subject") return } + switch comment.Subject.Collection() { + case tangled.RepoIssueNSID: + issues, err := db.GetIssues( + n.db, + orm.FilterEq("did", subjectDid), + orm.FilterEq("rkey", comment.Subject.RecordKey()), + ) + if err != nil { + l.Error("failed to get issues", "err", err) + return + } + if len(issues) == 0 { + l.Error("no issue found", "subject", comment.Subject) + return + } + issue := issues[0] + + recipients.Insert(syntax.DID(issue.Repo.Did)) + if comment.IsReply() { + // if this comment is a reply, then notify everybody in that thread + parentAtUri := *comment.ReplyTo + + // find the parent thread, and add all DIDs from here to the recipient list + for _, t := range issue.CommentList() { + if t.Self.AtUri() == parentAtUri { + for _, p := range t.Participants() { + recipients.Insert(p) + } + } + } + } else { + // not a reply, notify just the issue author + recipients.Insert(syntax.DID(issue.Did)) + } - // build the recipients list - // - owner of the repo - // - collaborators in the repo - // - remove users already mentioned - recipients := sets.Singleton(syntax.DID(issue.Repo.Did)) - for _, c := range collaborators { - recipients.Insert(c.SubjectDid) + entityType = "issue" + entityId = issue.AtUri().String() + repoId = &issue.Repo.Id + issueId = &issue.Id + case tangled.RepoPullNSID: + pulls, err := db.GetPulls( + n.db, + orm.FilterEq("owner_did", subjectDid), + orm.FilterEq("rkey", comment.Subject.RecordKey()), + ) + if err != nil { + l.Error("NewComment: failed to get pulls", "err", err) + return + } + if len(pulls) == 0 { + l.Error("NewComment: no pull found", "aturi", comment.Subject) + return + } + pull := pulls[0] + + pull.Repo, err = db.GetRepo(n.db, orm.FilterEq("at_uri", pull.RepoAt)) + if err != nil { + l.Error("NewComment: failed to get repo", "err", err) + return + } + + recipients.Insert(syntax.DID(pull.Repo.Did)) + for _, p := range pull.Participants() { + recipients.Insert(syntax.DID(p)) + } + + entityType = "pull" + entityId = pull.AtUri().String() + repoId = &pull.Repo.Id + p := int64(pull.ID) + pullId = &p + default: + return // no-op } - for _, m := range mentions { + + for _, m := range comment.Mentions { recipients.Remove(m) } - actorDid := syntax.DID(issue.Did) - entityType := "issue" - entityId := issue.AtUri().String() - repoId := &issue.Repo.Id - issueId := &issue.Id - var pullId *int64 - n.notifyEvent( ctx, - actorDid, + comment.Did, recipients, - models.NotificationTypeIssueCreated, + models.NotificationTypeIssueCommented, entityType, entityId, repoId, @@ -118,8 +191,8 @@ func (n *databaseNotifier) NewIssue(ctx context.Context, issue *models.Issue, me ) n.notifyEvent( ctx, - actorDid, - sets.Collect(slices.Values(mentions)), + comment.Did, + sets.Collect(slices.Values(comment.Mentions)), models.NotificationTypeUserMentioned, entityType, entityId, @@ -129,49 +202,32 @@ func (n *databaseNotifier) NewIssue(ctx context.Context, issue *models.Issue, me ) } -func (n *databaseNotifier) NewIssueComment(ctx context.Context, comment *models.Comment, mentions []syntax.DID) { +func (n *databaseNotifier) DeleteComment(ctx context.Context, comment *models.Comment) { + // no-op +} + +func (n *databaseNotifier) NewIssue(ctx context.Context, issue *models.Issue, mentions []syntax.DID) { l := log.FromContext(ctx) - issues, err := db.GetIssues(n.db, orm.FilterEq("at_uri", comment.Subject)) + collaborators, err := db.GetCollaborators(n.db, orm.FilterEq("repo_at", issue.Repo.RepoAt())) if err != nil { - l.Error("failed to get issues", "err", err) - return - } - if len(issues) == 0 { - l.Error("no issue found for", "err", comment.Subject) + l.Error("failed to fetch collaborators", "err", err) return } - issue := issues[0] - // built the recipients list: - // - the owner of the repo - // - | if the comment is a reply -> everybody on that thread - // | if the comment is a top level -> just the issue owner - // - remove mentioned users from the recipients list + // build the recipients list + // - owner of the repo + // - collaborators in the repo + // - remove users already mentioned recipients := sets.Singleton(syntax.DID(issue.Repo.Did)) - - if comment.IsReply() { - // if this comment is a reply, then notify everybody in that thread - parentAtUri := *comment.ReplyTo - - // find the parent thread, and add all DIDs from here to the recipient list - for _, t := range issue.CommentList() { - if t.Self.AtUri() == parentAtUri { - for _, p := range t.Participants() { - recipients.Insert(p) - } - } - } - } else { - // not a reply, notify just the issue author - recipients.Insert(syntax.DID(issue.Did)) + for _, c := range collaborators { + recipients.Insert(c.SubjectDid) } - for _, m := range mentions { recipients.Remove(m) } - actorDid := syntax.DID(comment.Did) + actorDid := syntax.DID(issue.Did) entityType := "issue" entityId := issue.AtUri().String() repoId := &issue.Repo.Id @@ -182,7 +238,7 @@ func (n *databaseNotifier) NewIssueComment(ctx context.Context, comment *models. ctx, actorDid, recipients, - models.NotificationTypeIssueCommented, + models.NotificationTypeIssueCreated, entityType, entityId, repoId, @@ -278,74 +334,6 @@ func (n *databaseNotifier) NewPull(ctx context.Context, pull *models.Pull) { ) } -func (n *databaseNotifier) NewPullComment(ctx context.Context, comment *models.Comment, mentions []syntax.DID) { - l := log.FromContext(ctx) - - pulls, err := db.GetPulls(n.db, - orm.FilterEq("owner_did", comment.Subject.Authority()), - orm.FilterEq("rkey", comment.Subject.RecordKey()), - ) - if err != nil { - l.Error("failed to get pull", "err", err) - return - } - if len(pulls) == 0 { - l.Error("NewPullComment: no pull found", "aturi", comment.Subject) - return - } - pull := pulls[0] - - repo, err := db.GetRepo(n.db, orm.FilterEq("at_uri", pull.RepoAt)) - if err != nil { - l.Error("failed to get repos", "err", err) - return - } - - // build up the recipients list: - // - repo owner - // - all pull participants - // - remove those already mentioned - recipients := sets.Singleton(syntax.DID(repo.Did)) - for _, p := range pull.Participants() { - recipients.Insert(syntax.DID(p)) - } - for _, m := range mentions { - recipients.Remove(m) - } - - actorDid := comment.Did - eventType := models.NotificationTypePullCommented - entityType := "pull" - entityId := pull.AtUri().String() - repoId := &repo.Id - var issueId *int64 - p := int64(pull.ID) - pullId := &p - - n.notifyEvent( - ctx, - actorDid, - recipients, - eventType, - entityType, - entityId, - repoId, - issueId, - pullId, - ) - n.notifyEvent( - ctx, - actorDid, - sets.Collect(slices.Values(mentions)), - models.NotificationTypeUserMentioned, - entityType, - entityId, - repoId, - issueId, - pullId, - ) -} - func (n *databaseNotifier) UpdateProfile(ctx context.Context, profile *models.Profile) { // no-op } diff --git a/appview/notify/logging_notifier.go b/appview/notify/logging_notifier.go index 59234281..015d58bd 100644 --- a/appview/notify/logging_notifier.go +++ b/appview/notify/logging_notifier.go @@ -44,9 +44,13 @@ func (l *loggingNotifier) NewIssue(ctx context.Context, issue *models.Issue, men l.inner.NewIssue(ctx, issue, mentions) } -func (l *loggingNotifier) NewIssueComment(ctx context.Context, comment *models.IssueComment, mentions []syntax.DID) { +func (l *loggingNotifier) NewComment(ctx context.Context, comment *models.Comment) { ctx = tlog.IntoContext(ctx, tlog.SubLogger(l.logger, "NewIssueComment")) - l.inner.NewIssueComment(ctx, comment, mentions) + l.inner.NewComment(ctx, comment) +} +func (l *loggingNotifier) DeleteComment(ctx context.Context, comment *models.Comment) { + ctx = tlog.IntoContext(ctx, tlog.SubLogger(l.logger, "DeleteComment")) + l.inner.DeleteComment(ctx, comment) } func (l *loggingNotifier) NewIssueState(ctx context.Context, actor syntax.DID, issue *models.Issue) { @@ -84,11 +88,6 @@ func (l *loggingNotifier) NewPull(ctx context.Context, pull *models.Pull) { l.inner.NewPull(ctx, pull) } -func (l *loggingNotifier) NewPullComment(ctx context.Context, comment *models.PullComment, mentions []syntax.DID) { - ctx = tlog.IntoContext(ctx, tlog.SubLogger(l.logger, "NewPullComment")) - l.inner.NewPullComment(ctx, comment, mentions) -} - func (l *loggingNotifier) NewPullState(ctx context.Context, actor syntax.DID, pull *models.Pull) { ctx = tlog.IntoContext(ctx, tlog.SubLogger(l.logger, "NewPullState")) l.inner.NewPullState(ctx, actor, pull) diff --git a/appview/notify/merged_notifier.go b/appview/notify/merged_notifier.go index 8e5a0813..496c3a89 100644 --- a/appview/notify/merged_notifier.go +++ b/appview/notify/merged_notifier.go @@ -42,12 +42,16 @@ func (m *mergedNotifier) DeleteStar(ctx context.Context, star *models.Star) { m.fanout(func(n Notifier) { n.DeleteStar(ctx, star) }) } -func (m *mergedNotifier) NewIssue(ctx context.Context, issue *models.Issue, mentions []syntax.DID) { - m.fanout(func(n Notifier) { n.NewIssue(ctx, issue, mentions) }) +func (m *mergedNotifier) NewComment(ctx context.Context, comment *models.Comment) { + m.fanout(func(n Notifier) { n.NewComment(ctx, comment) }) +} + +func (m *mergedNotifier) DeleteComment(ctx context.Context, comment *models.Comment) { + m.fanout(func(n Notifier) { n.DeleteComment(ctx, comment) }) } -func (m *mergedNotifier) NewIssueComment(ctx context.Context, comment *models.Comment, mentions []syntax.DID) { - m.fanout(func(n Notifier) { n.NewIssueComment(ctx, comment, mentions) }) +func (m *mergedNotifier) NewIssue(ctx context.Context, issue *models.Issue, mentions []syntax.DID) { + m.fanout(func(n Notifier) { n.NewIssue(ctx, issue, mentions) }) } func (m *mergedNotifier) NewIssueState(ctx context.Context, actor syntax.DID, issue *models.Issue) { @@ -78,10 +82,6 @@ func (m *mergedNotifier) NewPull(ctx context.Context, pull *models.Pull) { m.fanout(func(n Notifier) { n.NewPull(ctx, pull) }) } -func (m *mergedNotifier) NewPullComment(ctx context.Context, comment *models.Comment, mentions []syntax.DID) { - m.fanout(func(n Notifier) { n.NewPullComment(ctx, comment, mentions) }) -} - func (m *mergedNotifier) NewPullState(ctx context.Context, actor syntax.DID, pull *models.Pull) { m.fanout(func(n Notifier) { n.NewPullState(ctx, actor, pull) }) } diff --git a/appview/notify/notifier.go b/appview/notify/notifier.go index 6283babd..209e7db5 100644 --- a/appview/notify/notifier.go +++ b/appview/notify/notifier.go @@ -13,8 +13,10 @@ type Notifier interface { NewStar(ctx context.Context, star *models.Star) DeleteStar(ctx context.Context, star *models.Star) + NewComment(ctx context.Context, comment *models.Comment) + DeleteComment(ctx context.Context, comment *models.Comment) + NewIssue(ctx context.Context, issue *models.Issue, mentions []syntax.DID) - NewIssueComment(ctx context.Context, comment *models.Comment, mentions []syntax.DID) NewIssueState(ctx context.Context, actor syntax.DID, issue *models.Issue) DeleteIssue(ctx context.Context, issue *models.Issue) @@ -22,7 +24,6 @@ type Notifier interface { DeleteFollow(ctx context.Context, follow *models.Follow) NewPull(ctx context.Context, pull *models.Pull) - NewPullComment(ctx context.Context, comment *models.Comment, mentions []syntax.DID) NewPullState(ctx context.Context, actor syntax.DID, pull *models.Pull) NewIssueLabelOp(ctx context.Context, issue *models.Issue) @@ -47,9 +48,10 @@ func (m *BaseNotifier) NewRepo(ctx context.Context, repo *models.Repo) {} func (m *BaseNotifier) NewStar(ctx context.Context, star *models.Star) {} func (m *BaseNotifier) DeleteStar(ctx context.Context, star *models.Star) {} +func (m *BaseNotifier) NewComment(ctx context.Context, comment *models.Comment) {} +func (m *BaseNotifier) DeleteComment(ctx context.Context, comment *models.Comment) {} + func (m *BaseNotifier) NewIssue(ctx context.Context, issue *models.Issue, mentions []syntax.DID) {} -func (m *BaseNotifier) NewIssueComment(ctx context.Context, comment *models.Comment, mentions []syntax.DID) { -} func (m *BaseNotifier) NewIssueState(ctx context.Context, actor syntax.DID, issue *models.Issue) {} func (m *BaseNotifier) DeleteIssue(ctx context.Context, issue *models.Issue) {} @@ -59,9 +61,7 @@ func (m *BaseNotifier) NewPullLabelOp(ctx context.Context, pull *models.Pull) func (m *BaseNotifier) NewFollow(ctx context.Context, follow *models.Follow) {} func (m *BaseNotifier) DeleteFollow(ctx context.Context, follow *models.Follow) {} -func (m *BaseNotifier) NewPull(ctx context.Context, pull *models.Pull) {} -func (m *BaseNotifier) NewPullComment(ctx context.Context, models *models.Comment, mentions []syntax.DID) { -} +func (m *BaseNotifier) NewPull(ctx context.Context, pull *models.Pull) {} func (m *BaseNotifier) NewPullState(ctx context.Context, actor syntax.DID, pull *models.Pull) {} func (m *BaseNotifier) UpdateProfile(ctx context.Context, profile *models.Profile) {} diff --git a/appview/notify/posthog/notifier.go b/appview/notify/posthog/notifier.go index aa11523b..0562fcd1 100644 --- a/appview/notify/posthog/notifier.go +++ b/appview/notify/posthog/notifier.go @@ -86,20 +86,6 @@ func (n *posthogNotifier) NewPull(ctx context.Context, pull *models.Pull) { } } -func (n *posthogNotifier) NewPullComment(ctx context.Context, comment *models.Comment, mentions []syntax.DID) { - err := n.client.Enqueue(posthog.Capture{ - DistinctId: comment.Did.String(), - Event: "new_pull_comment", - Properties: posthog.Properties{ - "pull_at": comment.Subject, - "mentions": mentions, - }, - }) - if err != nil { - log.Println("failed to enqueue posthog event:", err) - } -} - func (n *posthogNotifier) NewPullClosed(ctx context.Context, pull *models.Pull) { err := n.client.Enqueue(posthog.Capture{ DistinctId: pull.OwnerDid, @@ -179,13 +165,13 @@ func (n *posthogNotifier) NewString(ctx context.Context, string *models.String) } } -func (n *posthogNotifier) NewIssueComment(ctx context.Context, comment *models.Comment, mentions []syntax.DID) { +func (n *posthogNotifier) NewComment(ctx context.Context, comment *models.Comment) { err := n.client.Enqueue(posthog.Capture{ DistinctId: comment.Did.String(), - Event: "new_issue_comment", + Event: "new_comment", Properties: posthog.Properties{ - "issue_at": comment.Subject, - "mentions": mentions, + "subject_at": comment.Subject, + "mentions": comment.Mentions, }, }) if err != nil { diff --git a/appview/pulls/pulls.go b/appview/pulls/pulls.go index d3ee2e7e..73f29b9b 100644 --- a/appview/pulls/pulls.go +++ b/appview/pulls/pulls.go @@ -888,7 +888,7 @@ func (s *Pulls) PullComment(w http.ResponseWriter, r *http.Request) { return } - s.notifier.NewPullComment(r.Context(), &comment, mentions) + s.notifier.NewComment(r.Context(), &comment) ownerSlashRepo := reporesolver.GetBaseRepoPath(r, f) s.pages.HxLocation(w, fmt.Sprintf("/%s/pulls/%d#comment-%d", ownerSlashRepo, pull.PullId, comment.Id)) -- 2.51.2