make hook summary abstract

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)
+			}
 		}
 	}
 

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

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()
 

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, &gts); 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.

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
 			}

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

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, &gts); 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"`
+}

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"}}
 

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

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"`
 }

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.
+}

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)
+}

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

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

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)
+}

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))
+		})
+	}
+}

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

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

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<string, string>
 }
 
+export interface DocumentHookSummaryGitHubTracking {
+	changedPaths: number
+}
+
 export interface DocumentHookSettingsURLWatcher {
 	url: string
 }

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 {