From 367138e5f3328873a840b71da260cfecd772f2d8 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 | 273 +++++++++++++++-------------- appview/notify/logging/notifier.go | 19 +- appview/notify/merged_notifier.go | 16 +- appview/notify/notifier.go | 15 +- appview/notify/posthog/notifier.go | 22 +-- appview/pulls/pulls.go | 2 +- 7 files changed, 168 insertions(+), 181 deletions(-) diff --git a/appview/issues/issues.go b/appview/issues/issues.go index 56b13b2e..d3689cb9 100644 --- a/appview/issues/issues.go +++ b/appview/issues/issues.go @@ -537,7 +537,7 @@ func (rp *Issues) NewIssueComment(w http.ResponseWriter, r *http.Request) { return } - rp.notifier.NewIssueComment(r.Context(), &comment, mentions) + rp.notifier.NewComment(r.Context(), &comment, mentions) 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 ee44999e..84909884 100644 --- a/appview/notify/db/db.go +++ b/appview/notify/db/db.go @@ -80,48 +80,135 @@ 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, mentions []syntax.DID) { l := log.FromContext(ctx) - collaborators, err := db.GetCollaborators(n.db, orm.FilterEq("repo_at", issue.Repo.RepoAt())) - if err != nil { - l.Error("failed to fetch collaborators", "err", err) - return - } + 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 + ) - // 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) - } - for _, m := range mentions { - recipients.Remove(m) - } + subjectAt := syntax.ATURI(comment.Subject.Uri) - actorDid := syntax.DID(issue.Did) - entityType := "issue" - entityId := issue.AtUri().String() - repoId := &issue.Repo.Id - issueId := &issue.Id - var pullId *int64 + switch subjectAt.Collection() { + case tangled.RepoIssueNSID: + issues, err := db.GetIssues( + n.db, + orm.FilterEq("at_uri", subjectAt), + ) + 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 + parent := *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() == syntax.ATURI(parent.Uri) { + for _, p := range t.Participants() { + recipients.Insert(p) + } + } + } + } else { + // not a reply, notify just the issue author + recipients.Insert(syntax.DID(issue.Did)) + } + + entityType = "issue" + entityId = issue.AtUri().String() + repoId = &issue.Repo.Id + issueId = &issue.Id + + for _, m := range mentions { + recipients.Remove(m) + } + + n.notifyEvent( + ctx, + comment.Did, + recipients, + models.NotificationTypeIssueCommented, + entityType, + entityId, + repoId, + issueId, + pullId, + ) + + case tangled.RepoPullNSID: + pulls, err := db.GetPulls( + n.db, + orm.FilterEq("owner_did", subjectAt.Authority()), + orm.FilterEq("rkey", subjectAt.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 + + for _, m := range mentions { + recipients.Remove(m) + } + + n.notifyEvent( + ctx, + comment.Did, + recipients, + models.NotificationTypePullCommented, + entityType, + entityId, + repoId, + issueId, + pullId, + ) + default: + return // no-op + } n.notifyEvent( ctx, - actorDid, - recipients, - models.NotificationTypeIssueCreated, - entityType, - entityId, - repoId, - issueId, - pullId, - ) - n.notifyEvent( - ctx, - actorDid, + comment.Did, sets.Collect(slices.Values(mentions)), models.NotificationTypeUserMentioned, entityType, @@ -132,49 +219,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 - parent := *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() == syntax.ATURI(parent.Uri) { - 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 @@ -185,7 +255,7 @@ func (n *databaseNotifier) NewIssueComment(ctx context.Context, comment *models. ctx, actorDid, recipients, - models.NotificationTypeIssueCommented, + models.NotificationTypeIssueCreated, entityType, entityId, repoId, @@ -281,75 +351,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) - - subjectAt := syntax.ATURI(comment.Subject.Uri) - pulls, err := db.GetPulls(n.db, - orm.FilterEq("owner_did", subjectAt.Authority()), - orm.FilterEq("rkey", subjectAt.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 874f2c26..6e771411 100644 --- a/appview/notify/logging/notifier.go +++ b/appview/notify/logging/notifier.go @@ -41,16 +41,20 @@ func (l *loggingNotifier) DeleteStar(ctx context.Context, star *models.Star) { l.inner.DeleteStar(ctx, star) } +func (l *loggingNotifier) NewComment(ctx context.Context, comment *models.Comment, mentions []syntax.DID) { + ctx = tlog.IntoContext(ctx, tlog.SubLogger(l.logger, "NewComment")) + l.inner.NewComment(ctx, comment, mentions) +} +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) NewIssue(ctx context.Context, issue *models.Issue, mentions []syntax.DID) { ctx = tlog.IntoContext(ctx, tlog.SubLogger(l.logger, "NewIssue")) l.inner.NewIssue(ctx, issue, mentions) } -func (l *loggingNotifier) NewIssueComment(ctx context.Context, comment *models.Comment, mentions []syntax.DID) { - ctx = tlog.IntoContext(ctx, tlog.SubLogger(l.logger, "NewIssueComment")) - l.inner.NewIssueComment(ctx, comment, mentions) -} - func (l *loggingNotifier) NewIssueState(ctx context.Context, actor syntax.DID, issue *models.Issue) { ctx = tlog.IntoContext(ctx, tlog.SubLogger(l.logger, "NewIssueState")) l.inner.NewIssueState(ctx, actor, issue) @@ -86,11 +90,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.Comment, 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 43cf0aca..a0f0be2b 100644 --- a/appview/notify/merged_notifier.go +++ b/appview/notify/merged_notifier.go @@ -46,12 +46,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, mentions []syntax.DID) { + m.fanout(func(n Notifier) { n.NewComment(ctx, comment, mentions) }) +} + +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) { @@ -82,10 +86,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 572832f8..15efa51e 100644 --- a/appview/notify/notifier.go +++ b/appview/notify/notifier.go @@ -14,8 +14,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, mentions []syntax.DID) + 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) @@ -23,7 +25,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) @@ -51,9 +52,11 @@ func (m *BaseNotifier) DeleteRepo(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) 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) NewComment(ctx context.Context, comment *models.Comment, mentions []syntax.DID) { } +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) NewIssueState(ctx context.Context, actor syntax.DID, issue *models.Issue) {} func (m *BaseNotifier) DeleteIssue(ctx context.Context, issue *models.Issue) {} @@ -63,9 +66,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 5f16e5dc..5ad61a18 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, @@ -190,13 +176,13 @@ func (n *posthogNotifier) Clone(ctx context.Context, repo *models.Repo) { } } -func (n *posthogNotifier) NewIssueComment(ctx context.Context, comment *models.Comment, mentions []syntax.DID) { +func (n *posthogNotifier) NewComment(ctx context.Context, comment *models.Comment, mentions []syntax.DID) { err := n.client.Enqueue(posthog.Capture{ DistinctId: comment.Did.String(), - Event: "new_issue_comment", + Event: "new_comment", Properties: posthog.Properties{ - "issue_at": comment.Subject.Uri, - "mentions": mentions, + "subject_at": comment.Subject.Uri, + "mentions": mentions, }, }) if err != nil { diff --git a/appview/pulls/pulls.go b/appview/pulls/pulls.go index 14a89521..64057b29 100644 --- a/appview/pulls/pulls.go +++ b/appview/pulls/pulls.go @@ -948,7 +948,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, mentions) ownerSlashRepo := reporesolver.GetBaseRepoPath(r, f) s.pages.HxLocation(w, fmt.Sprintf("/%s/pulls/%d#comment-%d", ownerSlashRepo, pull.PullId, comment.Id)) -- 2.51.2