diff --git a/server/core/internal/document/comment/comment.go b/server/core/internal/document/comment/comment.go index 6e167bde..0454c4fa 100644 --- a/server/core/internal/document/comment/comment.go +++ b/server/core/internal/document/comment/comment.go @@ -161,11 +161,12 @@ func writeText(sb *strings.Builder, node map[string]any) { return } - children, _ := node["content"].([]any) - - for _, child := range children { - if n, ok := child.(map[string]any); ok { - writeText(sb, n) + children, ok := node["content"].([]any) + if ok { + for _, child := range children { + if n, ok := child.(map[string]any); ok { + writeText(sb, n) + } } } diff --git a/server/core/internal/document/hook/hook.go b/server/core/internal/document/hook/hook.go index bd42e886..c3bb7414 100644 --- a/server/core/internal/document/hook/hook.go +++ b/server/core/internal/document/hook/hook.go @@ -275,6 +275,16 @@ func (h *Hook) Delete(ctx context.Context, inp *Input) error { return nil } +// Summary returns what the hook's last run found, in a shape its type +// defines. It is nil for a type that records nothing. +func (h *Hook) Summary() (any, error) { + if err := h.ensurePrepared(); err != nil { + return nil, err + } + + return h.runner.Summary(h.State.V) +} + // ChangedFrom reports whether the hook differs from prev in what editors // show of it. func (h *Hook) ChangedFrom(prev Hook) bool { @@ -439,4 +449,9 @@ type runner interface { // Delete cleans up any external resources associated with the hook. Delete(ctx context.Context, inp processor.Input) error + + // Summary should return what the given state records of the last run, + // in a shape the hook type defines, or nil for a type that records + // nothing. + Summary(state processor.State) (any, error) } diff --git a/server/core/internal/document/hook/hook_test.go b/server/core/internal/document/hook/hook_test.go index a95bcf7b..b4ea24e7 100644 --- a/server/core/internal/document/hook/hook_test.go +++ b/server/core/internal/document/hook/hook_test.go @@ -359,6 +359,62 @@ func Test_Hook_Delete(t *testing.T) { } } +func Test_Hook_Summary(t *testing.T) { + t.Parallel() + + cc := map[string]struct { + Hook Hook + Result any + Err error + }{ + "Malformed settings": { + Hook: Hook{ + Type: TypeGithubTracking, + Settings: processor.Settings(`{not json`), + }, + Err: assert.AnError, + }, + "Malformed state": { + Hook: Hook{ + Type: TypeGithubTracking, + Settings: processor.Settings(`{}`), + State: null.ValueFrom(processor.State(`{`)), + }, + Err: assert.AnError, + }, + "Hook type without a summary": { + Hook: Hook{ + Type: TypeURLWatcher, + Settings: processor.Settings(`{"url":"https://example.com"}`), + State: null.ValueFrom(processor.State(`{}`)), + }, + }, + "Successful summary": { + Hook: Hook{ + Type: TypeGithubTracking, + Settings: processor.Settings(`{}`), + State: null.ValueFrom(processor.State(`{"changedPaths":2}`)), + }, + Result: processor.GithubTrackingSummary{ChangedPaths: 2}, + }, + } + + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() + + res, err := c.Hook.Summary() + testutil.AssertEqualError(t, c.Err, err) + + if err != nil { + return + } + + assert.Equal(t, c.Result, res) + }) + } +} + func Test_Hook_ChangedFrom(t *testing.T) { t.Parallel() diff --git a/server/core/internal/document/hook/manager/manager.go b/server/core/internal/document/hook/manager/manager.go index f303ed53..46d398f6 100644 --- a/server/core/internal/document/hook/manager/manager.go +++ b/server/core/internal/document/hook/manager/manager.go @@ -3,7 +3,6 @@ package manager import ( "context" - "encoding/json" "fmt" "log/slog" "time" @@ -338,13 +337,23 @@ func (m *Manager) notifyTransition(ctx context.Context, prev, h hook.Hook) { // a scheduled reminder decays through 99…1, so the transition is the // arrival at zero, not a drop from full. case h.Status == processor.StatusActive && !prev.Score.IsZero() && h.Score.IsZero(): + summary, err := h.Summary() + if err != nil { + // the notification still goes out without the summary. + m.log.Warn( + "cannot summarize hook run", + slog.String("hook_id", h.ID.String()), + slog.String("error", err.Error()), + ) + } + core = notification.NewDocumentHookTriggeredNotification( h.DocumentID.V, h.Type, h.BlockID, h.BranchID.V, h.Settings, - m.changedPaths(h), + summary, ) default: return @@ -362,30 +371,6 @@ func (m *Manager) notifyTransition(ctx context.Context, prev, h hook.Hook) { m.notifPub.PublishNotifications(h.OrganizationID.String, core, maintainers...) } -// changedPaths returns how many tracked paths a GitHub hook found changed -// on its last run. It is null for any other hook, and for a state that -// does not decode. -func (m *Manager) changedPaths(h hook.Hook) null.Int { - if h.Type != hook.TypeGithubTracking { - return null.Int{} - } - - var gts processor.GithubTrackingState - - // the notification still goes out without the count. - if err := json.Unmarshal(h.State.V, >s); err != nil { - m.log.Warn( - "cannot decode github tracking state", - slog.String("hook_id", h.ID.String()), - slog.String("error", err.Error()), - ) - - return null.Int{} - } - - return null.IntFrom(int64(gts.ChangedPaths)) -} - // undoSetup tears down the resource h got in this run, since the row // naming it was not stored. Nothing else names it, so a failure is // reported. diff --git a/server/core/internal/document/hook/manager/manager_test.go b/server/core/internal/document/hook/manager/manager_test.go index 30248e14..ab15d6d9 100644 --- a/server/core/internal/document/hook/manager/manager_test.go +++ b/server/core/internal/document/hook/manager/manager_test.go @@ -900,11 +900,15 @@ func Test_Manager_notifyTransition(t *testing.T) { triggeredGithubHook.Score = decimal.Zero triggeredGithubHook.State = null.ValueFrom(processor.State(`{"pathsChecksums":{},"changedPaths":2}`)) + malformedGithubHook := triggeredGithubHook + malformedGithubHook.State = null.ValueFrom(processor.State(`{`)) + cc := map[string]struct { DB *DBMock Prev hook.Hook Hook hook.Hook Cores []notification.Core + Log string }{ "Hook that keeps its status and score": { DB: &DBMock{}, @@ -946,7 +950,7 @@ func Test_Manager_notifyTransition(t *testing.T) { null.String{}, branchID, urlHook.Settings, - null.Int{}, + nil, ), }, }, @@ -961,49 +965,25 @@ func Test_Manager_notifyTransition(t *testing.T) { null.String{}, branchID, githubHook.Settings, - null.IntFrom(2), + processor.GithubTrackingSummary{ChangedPaths: 2}, ), }, }, - } - - for cn, c := range cc { - t.Run(cn, func(t *testing.T) { - t.Parallel() - - pub := &fakePublisher{} - - newTestManager(t, c.DB, pub, nil).notifyTransition(context.Background(), c.Prev, c.Hook) - - assert.Equal(t, c.Cores, pub.cores) - }) - } -} - -func Test_Manager_changedPaths(t *testing.T) { - t.Parallel() - - cc := map[string]struct { - Hook hook.Hook - Result null.Int - Log string - }{ - "Hook of another type": { - Hook: urlWatcherHook(xid.New()), - }, - "Malformed state": { - Hook: hook.Hook{ - Type: hook.TypeGithubTracking, - State: null.ValueFrom(processor.State(`{`)), + "GitHub hook that ran out of freshness with a malformed state": { + DB: &DBMock{}, + Prev: githubHook, + Hook: malformedGithubHook, + Cores: []notification.Core{ + notification.NewDocumentHookTriggeredNotification( + urlHook.DocumentID.V, + hook.TypeGithubTracking, + null.String{}, + branchID, + githubHook.Settings, + nil, + ), }, - Log: "cannot decode github tracking state", - }, - "Successful count": { - Hook: hook.Hook{ - Type: hook.TypeGithubTracking, - State: null.ValueFrom(processor.State(`{"changedPaths":2}`)), - }, - Result: null.IntFrom(2), + Log: "cannot summarize hook run", }, } @@ -1013,13 +993,17 @@ func Test_Manager_changedPaths(t *testing.T) { var buf bytes.Buffer - man := newTestManager(t, &DBMock{}, &fakePublisher{}, nil) + pub := &fakePublisher{} + + man := newTestManager(t, c.DB, pub, nil) man.log = slog.New(slog.NewTextHandler(&buf, nil)) - assert.Equal(t, c.Result, man.changedPaths(c.Hook)) + man.notifyTransition(context.Background(), c.Prev, c.Hook) + + assert.Equal(t, c.Cores, pub.cores) if c.Log == "" { - assert.Empty(t, buf.String()) + assert.NotContains(t, buf.String(), "cannot summarize") return } diff --git a/server/core/internal/document/hook/processor/container_image_watcher.go b/server/core/internal/document/hook/processor/container_image_watcher.go index 884ef7c7..0258c51d 100644 --- a/server/core/internal/document/hook/processor/container_image_watcher.go +++ b/server/core/internal/document/hook/processor/container_image_watcher.go @@ -21,6 +21,7 @@ var ErrInvalidImage = errutil.New(http.StatusBadRequest, "document_hook.invalid_ // ContainerImageWatcher specifies a processor that watches container images for updates. type ContainerImageWatcher struct { PlainDeleter + PlainSummarizer // Image is the container image to watch. Image string `json:"image"` diff --git a/server/core/internal/document/hook/processor/github_tracking.go b/server/core/internal/document/hook/processor/github_tracking.go index 4b79de60..2e62e4a9 100644 --- a/server/core/internal/document/hook/processor/github_tracking.go +++ b/server/core/internal/document/hook/processor/github_tracking.go @@ -123,6 +123,17 @@ func (gt *GithubTracking) Reset(ctx context.Context, inp Input) (Result, error) return active(mathutil.Hundred, gts) } +// Summary returns how many tracked paths changed on the last run. +func (gt *GithubTracking) Summary(state State) (any, error) { + var gts GithubTrackingState + + if err := json.Unmarshal(state, >s); err != nil { + return nil, fmt.Errorf("unmarshaling github tracking state: %w", err) + } + + return GithubTrackingSummary{ChangedPaths: gts.ChangedPaths}, nil +} + // fetchTree resolves the GitHub client and pulls the tracked repository's // tree. A status other than active means the tree could not be read for a // reason the hook reports rather than fails on. @@ -166,3 +177,10 @@ type GithubTrackingState struct { // checksums on the last run. ChangedPaths int `json:"changedPaths"` } + +// GithubTrackingSummary represents what the GitHub tracking processor +// found on its last run. +type GithubTrackingSummary struct { + // ChangedPaths specifies the number of tracked paths that changed. + ChangedPaths int `json:"changedPaths"` +} diff --git a/server/core/internal/document/hook/processor/github_tracking_test.go b/server/core/internal/document/hook/processor/github_tracking_test.go index aaa12020..1b61270c 100644 --- a/server/core/internal/document/hook/processor/github_tracking_test.go +++ b/server/core/internal/document/hook/processor/github_tracking_test.go @@ -278,6 +278,40 @@ func (i githubTreeInput) ChangeDetection() ChangeDetection { return nil } +func Test_GithubTracking_Summary(t *testing.T) { + t.Parallel() + + cc := map[string]struct { + State State + Result any + Err error + }{ + "Malformed state": { + State: State(`{`), + Err: assert.AnError, + }, + "Successful summary": { + State: State(`{"pathsChecksums":{"a.md":"x"},"changedPaths":2}`), + Result: GithubTrackingSummary{ChangedPaths: 2}, + }, + } + + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() + + res, err := (&GithubTracking{}).Summary(c.State) + testutil.AssertEqualError(t, c.Err, err) + + if err != nil { + return + } + + assert.Equal(t, c.Result, res) + }) + } +} + func Test_GithubTracking_fetchTree(t *testing.T) { tree := github.Tree{{Name: "doc.md", Checksum: "sum"}} diff --git a/server/core/internal/document/hook/processor/scheduled_reminder.go b/server/core/internal/document/hook/processor/scheduled_reminder.go index b58718e0..42135449 100644 --- a/server/core/internal/document/hook/processor/scheduled_reminder.go +++ b/server/core/internal/document/hook/processor/scheduled_reminder.go @@ -37,6 +37,7 @@ const _scheduleGracePeriod = time.Second * 5 // score based on a scheduled time. type ScheduledReminder struct { PlainDeleter + PlainSummarizer // Scale is the type of scale used for scoring. Scale ScaleType `json:"scale"` diff --git a/server/core/internal/document/hook/processor/url_watcher.go b/server/core/internal/document/hook/processor/url_watcher.go index ada66705..2dd469da 100644 --- a/server/core/internal/document/hook/processor/url_watcher.go +++ b/server/core/internal/document/hook/processor/url_watcher.go @@ -21,6 +21,8 @@ var ErrInvalidURL = errutil.New(http.StatusBadRequest, "document_hook.invalid_ur // URLWatcher specifies a processor that watches a URL for changes. type URLWatcher struct { + PlainSummarizer + // URL is the URL to watch. URL string `json:"url"` } diff --git a/server/core/internal/document/hook/processor/util.go b/server/core/internal/document/hook/processor/util.go index b7d5afd4..bc3d42dc 100644 --- a/server/core/internal/document/hook/processor/util.go +++ b/server/core/internal/document/hook/processor/util.go @@ -9,3 +9,11 @@ type PlainDeleter struct{} func (*PlainDeleter) Delete(_ context.Context, _ Input) error { return nil } + +// PlainSummarizer is a summarizer that reports nothing. +type PlainSummarizer struct{} + +// Summary always returns nil. +func (*PlainSummarizer) Summary(_ State) (any, error) { + return nil, nil //nolint:nilnil // a processor without a summary has nothing to return. +} diff --git a/server/core/internal/document/hook/processor/util_test.go b/server/core/internal/document/hook/processor/util_test.go index 0b5af16b..66dc2dc5 100644 --- a/server/core/internal/document/hook/processor/util_test.go +++ b/server/core/internal/document/hook/processor/util_test.go @@ -31,3 +31,11 @@ func Test_PlainDeleter_Delete(t *testing.T) { assert.NoError(t, (&PlainDeleter{}).Delete(context.Background(), stubInput{})) } + +func Test_PlainSummarizer_Summary(t *testing.T) { + t.Parallel() + + res, err := (&PlainSummarizer{}).Summary(State(`{}`)) + assert.NoError(t, err) + assert.Nil(t, res) +} diff --git a/server/core/internal/notification/codes.go b/server/core/internal/notification/codes.go index 40b598e9..88d21aa6 100644 --- a/server/core/internal/notification/codes.go +++ b/server/core/internal/notification/codes.go @@ -15,18 +15,18 @@ type Code string // a stored notification must name keys through these constants too — a raw // literal would compile clean and silently drift on a rename. const ( - MetaKeyUserID = "userId" - MetaKeyDocumentID = "documentId" - MetaKeyBranchID = "branchId" - MetaKeyBlockID = "blockId" - MetaKeyType = "type" - MetaKeyCommentID = "commentId" - MetaKeyCommentReplyID = "commentReplyId" - MetaKeyAnchorBlockID = "anchorBlockId" - MetaKeyStatus = "status" - MetaKeyHookSettings = "hookSettings" - MetaKeyGithubTrackingChangedPaths = "githubTrackingChangedPaths" - MetaKeyCommentExcerpt = "commentExcerpt" + MetaKeyUserID = "userId" + MetaKeyDocumentID = "documentId" + MetaKeyBranchID = "branchId" + MetaKeyBlockID = "blockId" + MetaKeyType = "type" + MetaKeyCommentID = "commentId" + MetaKeyCommentReplyID = "commentReplyId" + MetaKeyAnchorBlockID = "anchorBlockId" + MetaKeyStatus = "status" + MetaKeyHookSettings = "hookSettings" + MetaKeyHookSummary = "hookSummary" + MetaKeyCommentExcerpt = "commentExcerpt" ) const ( @@ -63,25 +63,25 @@ func NewDocumentReviewRequestNotification(userID string, documentID, branchID xi } // NewDocumentHookTriggeredNotification creates a new document hook triggered -// notification core. It keeps the settings the hook had when it triggered. -// The changed paths are null for a hook that counts none. +// notification core. It keeps the settings the hook had when it triggered +// and what its last run found, in a shape the hook type defines. func NewDocumentHookTriggeredNotification( documentID xid.ID, tp hook.Type, blockID null.String, branchID xid.ID, settings processor.Settings, - changedPaths null.Int, + summary any, ) Core { return Core{ Code: NotificationDocumentHookTriggered, Metadata: map[string]any{ - MetaKeyDocumentID: documentID, - MetaKeyBlockID: blockID, - MetaKeyType: tp, - MetaKeyBranchID: branchID, - MetaKeyHookSettings: settings, - MetaKeyGithubTrackingChangedPaths: changedPaths, + MetaKeyDocumentID: documentID, + MetaKeyBlockID: blockID, + MetaKeyType: tp, + MetaKeyBranchID: branchID, + MetaKeyHookSettings: settings, + MetaKeyHookSummary: summary, }, } } diff --git a/server/core/internal/notification/codes_test.go b/server/core/internal/notification/codes_test.go index 238d0ce1..0f342bee 100644 --- a/server/core/internal/notification/codes_test.go +++ b/server/core/internal/notification/codes_test.go @@ -38,18 +38,18 @@ func Test_NewDocumentHookTriggeredNotification(t *testing.T) { null.StringFrom("blk1"), branchID, processor.Settings(`{"repository":"repo"}`), - null.IntFrom(2), + processor.GithubTrackingSummary{ChangedPaths: 2}, ) assert.Equal(t, Core{ Code: NotificationDocumentHookTriggered, Metadata: Metadata{ - MetaKeyDocumentID: documentID, - MetaKeyBlockID: null.StringFrom("blk1"), - MetaKeyType: hook.TypeGithubTracking, - MetaKeyBranchID: branchID, - MetaKeyHookSettings: processor.Settings(`{"repository":"repo"}`), - MetaKeyGithubTrackingChangedPaths: null.IntFrom(2), + MetaKeyDocumentID: documentID, + MetaKeyBlockID: null.StringFrom("blk1"), + MetaKeyType: hook.TypeGithubTracking, + MetaKeyBranchID: branchID, + MetaKeyHookSettings: processor.Settings(`{"repository":"repo"}`), + MetaKeyHookSummary: processor.GithubTrackingSummary{ChangedPaths: 2}, }, }, nc) } diff --git a/server/core/pkg/strutil/strutil.go b/server/core/pkg/strutil/strutil.go new file mode 100644 index 00000000..d97e513c --- /dev/null +++ b/server/core/pkg/strutil/strutil.go @@ -0,0 +1,28 @@ +package strutil + +import "strings" + +// Ellipsize returns at most maxLen runes of s, appending a single +// ellipsis rune when the input was longer. A non-positive maxLen +// returns s unchanged. +func Ellipsize(s string, maxLen int) string { + if maxLen <= 0 { + return s + } + + runes := []rune(s) + if len(runes) <= maxLen { + return s + } + + return string(runes[:maxLen]) + "…" +} + +// Preview returns at most maxLen runes of s, collapsed to a single line +// and trimmed, elided the way Ellipsize does. +func Preview(s string, maxLen int) string { + s = strings.ReplaceAll(s, "\n", " ") + s = strings.TrimSpace(s) + + return Ellipsize(s, maxLen) +} diff --git a/server/core/pkg/strutil/strutil_test.go b/server/core/pkg/strutil/strutil_test.go new file mode 100644 index 00000000..ae9a8a5c --- /dev/null +++ b/server/core/pkg/strutil/strutil_test.go @@ -0,0 +1,52 @@ +package strutil + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func Test_Ellipsize(t *testing.T) { + cc := map[string]struct { + Input string + MaxLen int + Result string + }{ + "Shorter than limit": {Input: "abc", MaxLen: 5, Result: "abc"}, + "Exactly at limit": {Input: "abcde", MaxLen: 5, Result: "abcde"}, + "Longer than limit": {Input: "abcdef", MaxLen: 5, Result: "abcde…"}, + "Zero limit passthrough": {Input: "abcdef", MaxLen: 0, Result: "abcdef"}, + "Negative limit": {Input: "abcdef", MaxLen: -1, Result: "abcdef"}, + "Multibyte runes": {Input: "ąčęėįš", MaxLen: 3, Result: "ąčę…"}, + "Empty input": {Input: "", MaxLen: 3, Result: ""}, + } + + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() + + assert.Equal(t, c.Result, Ellipsize(c.Input, c.MaxLen)) + }) + } +} + +func Test_Preview(t *testing.T) { + cc := map[string]struct { + Input string + MaxLen int + Result string + }{ + "Newlines collapse": {Input: "a\nb\nc", MaxLen: 10, Result: "a b c"}, + "Whitespace trimmed": {Input: " x ", MaxLen: 10, Result: "x"}, + "Long text elided": {Input: "abcdef", MaxLen: 3, Result: "abc…"}, + "Empty input": {Input: "", MaxLen: 3, Result: ""}, + } + + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() + + assert.Equal(t, c.Result, Preview(c.Input, c.MaxLen)) + }) + } +} diff --git a/web/app/components/notification.test.ts b/web/app/components/notification.test.ts index 7037ec94..cf8cd9fd 100644 --- a/web/app/components/notification.test.ts +++ b/web/app/components/notification.test.ts @@ -123,7 +123,7 @@ describe("hookNotificationLine", () => { branch: "main", paths: ["a.md", "b.md", "c.md"], }, - githubTrackingChangedPaths: 2, + hookSummary: { changedPaths: 2 }, }, expected: { text: 'notification.messages.hook-triggered.github-tracking:{"changed":2,"total":3}', @@ -139,7 +139,7 @@ describe("hookNotificationLine", () => { branch: "main", paths: ["a.md"], }, - githubTrackingChangedPaths: null, + hookSummary: null, }, expected: { text: "notification.messages.hook-triggered.github-tracking-fallback", diff --git a/web/app/components/notification.ts b/web/app/components/notification.ts index ba617186..572e909f 100644 --- a/web/app/components/notification.ts +++ b/web/app/components/notification.ts @@ -125,9 +125,9 @@ function triggeredLine( metadata.hookSettings as DocumentHookSettingsGitHubTracking return { - text: metadata.githubTrackingChangedPaths + text: metadata.hookSummary?.changedPaths ? t("notification.messages.hook-triggered.github-tracking", { - changed: metadata.githubTrackingChangedPaths, + changed: metadata.hookSummary.changedPaths, total: settings.paths.length, }) : t("notification.messages.hook-triggered.github-tracking-fallback"), diff --git a/web/app/utils/api/document.ts b/web/app/utils/api/document.ts index da921dfb..6322ce12 100644 --- a/web/app/utils/api/document.ts +++ b/web/app/utils/api/document.ts @@ -114,6 +114,7 @@ export type DocumentHookState = | DocumentHookStateGitHubTracking | DocumentHookStateURLWatcher | DocumentHookStateContainerImageWatcher +export type DocumentHookSummary = DocumentHookSummaryGitHubTracking export type DocumentHookStatus = | DocumentHookStatusScheduledReminder | DocumentHookStatusGitHubTracking @@ -155,6 +156,10 @@ export interface DocumentHookStateGitHubTracking { pathsChecksums: Record } +export interface DocumentHookSummaryGitHubTracking { + changedPaths: number +} + export interface DocumentHookSettingsURLWatcher { url: string } diff --git a/web/app/utils/api/notification.ts b/web/app/utils/api/notification.ts index 5ed804fe..b130734b 100644 --- a/web/app/utils/api/notification.ts +++ b/web/app/utils/api/notification.ts @@ -46,7 +46,7 @@ export interface NotificationMetadataDocumentHookTriggered { type: DocumentHookType hookSettings?: DocumentHookSettings | null status?: DocumentHookStatus - githubTrackingChangedPaths?: number | null + hookSummary?: DocumentHookSummary | null } export interface NotificationMetadataDocumentNewComment {