diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index aff88078..4fbb041d 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -234,7 +234,8 @@ gating in step 3. `web/app/components/editor/hooks/`, then follow an existing type through the menu content beside it (the component and props switches and the add-menu entry, gated on the capability when the hook depends - on one) and through the notification box in `web/app/components/`. + on one) and through `NotificationRow.vue` and `notification.ts` in + `web/app/components/`. Their strings live in `web/i18n/locales/`. 6. **Extend the tests.** Every place above that switches on the type has a table-driven test with one row per type; extend the tables in the diff --git a/Makefile b/Makefile index 29ecce53..98cae986 100644 --- a/Makefile +++ b/Makefile @@ -128,13 +128,11 @@ test: @$(QUIET) "testing launcher" sh -c 'cd docker/prod/launcher && pnpm run test' # everything CI checks, to run before a commit. Nothing here modifies a -# file. The steps are recipe lines so that `make -j` cannot start both e2e -# stacks at once. +# file. .PHONY: check-all check-all: @$(MAKE) check-lint @$(MAKE) test - @$(MAKE) e2e-dev @$(MAKE) e2e-prod .PHONY: setup diff --git a/e2e/helpers/inbox.ts b/e2e/helpers/inbox.ts index 4a7ac659..53b2303f 100644 --- a/e2e/helpers/inbox.ts +++ b/e2e/helpers/inbox.ts @@ -11,7 +11,11 @@ export async function openInbox(page: Page): Promise { }) .click() + // the closed inbox stays mounted beside the screen, so only its place + // tells that it opened await expect( - page.getByRole("button", { name: t("notification.read-all-button") }), - ).toBeVisible() + page.getByRole("button", { + name: t("notification.actions.close-notification-box"), + }), + ).toBeInViewport() } diff --git a/e2e/tests/comments.test.ts b/e2e/tests/comments.test.ts index 2af68b0c..83ea5623 100644 --- a/e2e/tests/comments.test.ts +++ b/e2e/tests/comments.test.ts @@ -126,13 +126,13 @@ test.describe("comments", () => { await expect(linked).not.toBeInViewport() await openInbox(page) - await page - .getByRole("link", { - name: t("notification.messages.document-new-comment-description", { - user: other.credentials.email.split("@")[0] ?? "", - }), - }) - .click() + const notification = page.getByRole("link", { + name: t("notification.messages.document-new-comment-description", { + user: other.credentials.email.split("@")[0] ?? "", + }), + }) + await expect(notification).toContainText("Worth a second look") + await notification.click() await expect(page).toHaveURL( new RegExp(`^${url}\\?branch=[a-z0-9]{20}#${uid}$`), @@ -142,6 +142,46 @@ test.describe("comments", () => { await other.context.close() }) + test("tells the author in the inbox when a teammate resolves their comment", async ({ + page, + request, + browser, + }) => { + // two signups, a verification each, an invitation and its + // acceptance run before the first assertion + test.slow() + + await signUpWithWorkspace(page, request) + await createDocument(page) + await contentEditor(page).click() + await page.keyboard.type("Soon to be settled") + const other = await joinAsSecondUser(browser, page, request) + // the owner's member list predates the teammate, so a reload picks + // them up before the notification names them + await visit(page, page.url()) + await waitForEditor(page) + await addComment(page, "Handled already") + + await contentEditor(other.page).locator(".comment-mark").click() + await commentPopover(other.page) + .getByRole("button", { + name: t("editor.comment-thread.resolve-button"), + exact: true, + }) + .click() + + await openInbox(page) + await expect( + page.getByRole("link", { + name: t("notification.messages.document-comment-resolved-description", { + user: other.credentials.email.split("@")[0] ?? "", + }), + }), + ).toContainText("Handled already") + + await other.context.close() + }) + test("replies to a comment", async ({ page, request }) => { await signUpWithWorkspace(page, request) await createDocument(page) diff --git a/e2e/tests/review.test.ts b/e2e/tests/review.test.ts index bacd6ae9..9d45c62d 100644 --- a/e2e/tests/review.test.ts +++ b/e2e/tests/review.test.ts @@ -474,7 +474,7 @@ test.describe("review workflow", () => { request, browser, }) => { - await signUpWithWorkspace(page, request) + const { credentials } = await signUpWithWorkspace(page, request) await createDocument(page) const other = await joinAsSecondUser(browser, page, request) // the owner's member list predates the teammate, so a reload picks @@ -489,7 +489,9 @@ test.describe("review workflow", () => { await expect( other.page.getByText( - t("notification.messages.document-review-request-description"), + t("notification.messages.document-review-request-description", { + user: credentials.email.split("@")[0] ?? "", + }), ), ).toBeVisible() diff --git a/server/core/internal/document/comment/comment.go b/server/core/internal/document/comment/comment.go index 9541f69b..6e167bde 100644 --- a/server/core/internal/document/comment/comment.go +++ b/server/core/internal/document/comment/comment.go @@ -7,10 +7,12 @@ import ( "fmt" "net/http" "slices" + "strings" "time" "github.com/guregu/null/v5" "github.com/oxynote/oxynote/server/core/pkg/errutil" + "github.com/oxynote/oxynote/server/core/pkg/strutil" "github.com/oxynote/oxynote/server/core/pkg/timeutil" "github.com/rs/xid" ) @@ -23,6 +25,9 @@ var ( ErrMissingStatus = errutil.New(http.StatusBadRequest, "comment.missing_status", "comment resolved status is required") ) +// _excerptLength is the most runes an excerpt keeps of a comment's text. +const _excerptLength = 120 + // Comment represents a comment on a document. type Comment struct { // ID is the unique identifier for the comment. @@ -137,6 +142,36 @@ func (c *Content) Scan(value any) error { return nil } +// Excerpt returns the start of the content's text on a single line. +func (c Content) Excerpt() string { + var sb strings.Builder + + writeText(&sb, c) + + return strutil.Preview(strings.Join(strings.Fields(sb.String()), " "), _excerptLength) +} + +// writeText appends the text of a node of the content and of its children. +// Text runs within a node join as written, and a space follows every other +// node. +func writeText(sb *strings.Builder, node map[string]any) { + if text, ok := node["text"].(string); ok { + sb.WriteString(text) + + return + } + + children, _ := node["content"].([]any) + + for _, child := range children { + if n, ok := child.(map[string]any); ok { + writeText(sb, n) + } + } + + sb.WriteByte(' ') +} + // NewComment creates a new comment instance with the provided input. func NewComment(inp Input, documentID, branchID xid.ID, userID, organizationID string) Comment { return Comment{ diff --git a/server/core/internal/document/comment/comment_test.go b/server/core/internal/document/comment/comment_test.go index 0464f93d..b8951f5d 100644 --- a/server/core/internal/document/comment/comment_test.go +++ b/server/core/internal/document/comment/comment_test.go @@ -2,6 +2,7 @@ package comment import ( "encoding/json" + "strings" "testing" "time" @@ -329,3 +330,91 @@ func Test_Content_Value(t *testing.T) { assert.Nil(t, val) }) } + +func Test_Content_Excerpt(t *testing.T) { + t.Parallel() + + paragraph := func(texts ...string) map[string]any { + nodes := make([]any, 0, len(texts)) + + for _, text := range texts { + nodes = append(nodes, map[string]any{"type": "text", "text": text}) + } + + return map[string]any{"type": "paragraph", "content": nodes} + } + + cc := map[string]struct { + Content Content + Result string + }{ + "Content without text is empty": { + Content: Content{"type": "doc"}, + }, + "Marked runs join and blocks are set apart": { + Content: Content{ + "type": "doc", + "content": []any{ + paragraph("Worth a ", "second", " look"), + paragraph("before the release"), + }, + }, + Result: "Worth a second look before the release", + }, + "Nested blocks and breaks are set apart once": { + Content: Content{ + "type": "doc", + "content": []any{ + map[string]any{ + "type": "bulletList", + "content": []any{ + map[string]any{"type": "listItem", "content": []any{paragraph("first")}}, + map[string]any{"type": "listItem", "content": []any{paragraph("second")}}, + }, + }, + map[string]any{ + "type": "paragraph", + "content": []any{ + map[string]any{"type": "text", "text": "third"}, + map[string]any{"type": "hardBreak"}, + map[string]any{"type": "text", "text": "fourth"}, + "stray", + }, + }, + }, + }, + Result: "first second third fourth", + }, + "Long text is cut by runes": { + Content: Content{ + "type": "doc", + "content": []any{paragraph(strings.Repeat("ž", _excerptLength+1))}, + }, + Result: strings.Repeat("ž", _excerptLength) + "…", + }, + } + + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() + + assert.Equal(t, c.Result, c.Content.Excerpt()) + }) + } +} + +func Test_writeText(t *testing.T) { + t.Parallel() + + var sb strings.Builder + + writeText(&sb, map[string]any{ + "type": "paragraph", + "content": []any{ + map[string]any{"type": "text", "text": "one"}, + map[string]any{"type": "text", "text": "two"}, + }, + }) + + assert.Equal(t, "onetwo ", sb.String()) +} diff --git a/server/core/internal/document/hook/manager/manager.go b/server/core/internal/document/hook/manager/manager.go index 5ae51e74..f303ed53 100644 --- a/server/core/internal/document/hook/manager/manager.go +++ b/server/core/internal/document/hook/manager/manager.go @@ -3,6 +3,7 @@ package manager import ( "context" + "encoding/json" "fmt" "log/slog" "time" @@ -332,6 +333,7 @@ func (m *Manager) notifyTransition(ctx context.Context, prev, h hook.Hook) { h.BlockID, h.BranchID.V, h.Status, + h.Settings, ) // a scheduled reminder decays through 99…1, so the transition is the // arrival at zero, not a drop from full. @@ -341,6 +343,8 @@ func (m *Manager) notifyTransition(ctx context.Context, prev, h hook.Hook) { h.Type, h.BlockID, h.BranchID.V, + h.Settings, + m.changedPaths(h), ) default: return @@ -358,6 +362,30 @@ 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 ceec2a7b..30248e14 100644 --- a/server/core/internal/document/hook/manager/manager_test.go +++ b/server/core/internal/document/hook/manager/manager_test.go @@ -1,6 +1,7 @@ package manager import ( + "bytes" "context" "database/sql" "encoding/json" @@ -38,6 +39,7 @@ type fakePublisher struct { organizationIDs []string codes []notification.Code + cores []notification.Core userIDs [][]string } @@ -47,6 +49,7 @@ func (f *fakePublisher) PublishNotifications(organizationID string, core notific f.organizationIDs = append(f.organizationIDs, organizationID) f.codes = append(f.codes, core.Code) + f.cores = append(f.cores, core) f.userIDs = append(f.userIDs, userIDs) } @@ -876,6 +879,156 @@ func Test_Manager_processHooks(t *testing.T) { } } +func Test_Manager_notifyTransition(t *testing.T) { + t.Parallel() + + branchID := xid.New() + + urlHook := urlWatcherHook(branchID) + + failedHook := urlHook + failedHook.Status = processor.StatusUnreachableURL + + triggeredHook := urlHook + triggeredHook.Score = decimal.Zero + + githubHook := urlHook + githubHook.Type = hook.TypeGithubTracking + githubHook.Settings = processor.Settings(`{"repository":"repo","branch":"main","paths":["a.md","b.md","c.md"]}`) + + triggeredGithubHook := githubHook + triggeredGithubHook.Score = decimal.Zero + triggeredGithubHook.State = null.ValueFrom(processor.State(`{"pathsChecksums":{},"changedPaths":2}`)) + + cc := map[string]struct { + DB *DBMock + Prev hook.Hook + Hook hook.Hook + Cores []notification.Core + }{ + "Hook that keeps its status and score": { + DB: &DBMock{}, + Prev: urlHook, + Hook: urlHook, + }, + "Error returned by DB.FetchDocumentMaintainers": { + DB: &DBMock{ + FetchDocumentMaintainersFunc: func(context.Context, xid.ID, string) ([]string, error) { + return nil, assert.AnError + }, + }, + Prev: urlHook, + Hook: triggeredHook, + }, + "Hook that can no longer check its target": { + DB: &DBMock{}, + Prev: urlHook, + Hook: failedHook, + Cores: []notification.Core{ + notification.NewDocumentHookNeedsAttentionNotification( + urlHook.DocumentID.V, + hook.TypeURLWatcher, + null.String{}, + branchID, + processor.StatusUnreachableURL, + urlHook.Settings, + ), + }, + }, + "Hook that ran out of freshness": { + DB: &DBMock{}, + Prev: urlHook, + Hook: triggeredHook, + Cores: []notification.Core{ + notification.NewDocumentHookTriggeredNotification( + urlHook.DocumentID.V, + hook.TypeURLWatcher, + null.String{}, + branchID, + urlHook.Settings, + null.Int{}, + ), + }, + }, + "GitHub hook that ran out of freshness": { + DB: &DBMock{}, + Prev: githubHook, + Hook: triggeredGithubHook, + Cores: []notification.Core{ + notification.NewDocumentHookTriggeredNotification( + urlHook.DocumentID.V, + hook.TypeGithubTracking, + null.String{}, + branchID, + githubHook.Settings, + null.IntFrom(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(`{`)), + }, + 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), + }, + } + + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() + + var buf bytes.Buffer + + man := newTestManager(t, &DBMock{}, &fakePublisher{}, nil) + man.log = slog.New(slog.NewTextHandler(&buf, nil)) + + assert.Equal(t, c.Result, man.changedPaths(c.Hook)) + + if c.Log == "" { + assert.Empty(t, buf.String()) + + return + } + + assert.Contains(t, buf.String(), c.Log) + }) + } +} + func Test_Manager_input(t *testing.T) { t.Parallel() diff --git a/server/core/internal/document/hook/processor/github_tracking.go b/server/core/internal/document/hook/processor/github_tracking.go index f0900e65..4b79de60 100644 --- a/server/core/internal/document/hook/processor/github_tracking.go +++ b/server/core/internal/document/hook/processor/github_tracking.go @@ -52,7 +52,8 @@ func (gt *GithubTracking) Validate() error { return nil } -// Process scores the hook down once a tracked path changed since the reset. +// Process scores the hook down once a tracked path changed since the reset, +// and records how many did. func (gt *GithubTracking) Process(ctx context.Context, inp Input) (Result, error) { var gts GithubTrackingState @@ -69,7 +70,7 @@ func (gt *GithubTracking) Process(ctx context.Context, inp Input) (Result, error return inactive(status), nil } - score := mathutil.Hundred + gts.ChangedPaths = 0 for _, path := range gt.Paths { item, ok := tree.GetItem(path) @@ -79,11 +80,16 @@ func (gt *GithubTracking) Process(ctx context.Context, inp Input) (Result, error // map, so a path missing both then and now is unchanged; only a // presence flip or a differing checksum marks a modification. if ok != tracked || (ok && item.Checksum != checksum) { - score = decimal.Zero - break + gts.ChangedPaths++ } } + score := mathutil.Hundred + + if gts.ChangedPaths > 0 { + score = decimal.Zero + } + return active(score, gts) } @@ -155,4 +161,8 @@ func (gt *GithubTracking) fetchTree(ctx context.Context, inp Input) (github.Tree type GithubTrackingState struct { // PathsChecksums is a map of file paths to their checksums. PathsChecksums map[string]string `json:"pathsChecksums"` + + // ChangedPaths is the number of tracked paths that differed from the + // checksums on the last run. + 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 3a27e837..aaa12020 100644 --- a/server/core/internal/document/hook/processor/github_tracking_test.go +++ b/server/core/internal/document/hook/processor/github_tracking_test.go @@ -2,6 +2,7 @@ package processor import ( "context" + "encoding/json" "testing" "github.com/oxynote/oxynote/server/core/internal/apps/github" @@ -9,6 +10,7 @@ import ( "github.com/oxynote/oxynote/server/core/pkg/testutil" "github.com/shopspring/decimal" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) // githubErrInput is a test Input whose Github method always fails with the @@ -67,10 +69,12 @@ func Test_GithubTracking_Process(t *testing.T) { t.Parallel() cc := map[string]struct { - Inp Input - WantScore decimal.Decimal - WantStatus Status - WantErr bool + Inp Input + Paths []string + WantScore decimal.Decimal + WantStatus Status + WantChanged int + WantErr bool }{ "Installation not found is a status": { Inp: githubErrInput{state: State("{}"), err: github.ErrInstallationNotFound}, @@ -101,6 +105,27 @@ func Test_GithubTracking_Process(t *testing.T) { state: State(`{"pathsChecksums":{"doc.md":"old"}}`), client: githubTreeClient{tree: github.Tree{{Name: "doc.md", Checksum: "sum"}}}, }, + WantStatus: StatusActive, + WantChanged: 1, + }, + "Every changed path is counted": { + Inp: githubTreeInput{ + state: State(`{"pathsChecksums":{"doc.md":"old","api.md":"sum","gone.md":"sum"}}`), + client: githubTreeClient{tree: github.Tree{ + {Name: "doc.md", Checksum: "sum"}, + {Name: "api.md", Checksum: "sum"}, + }}, + }, + Paths: []string{"doc.md", "api.md", "gone.md"}, + WantStatus: StatusActive, + WantChanged: 2, + }, + "Count from the previous run is replaced": { + Inp: githubTreeInput{ + state: State(`{"pathsChecksums":{"doc.md":"sum"},"changedPaths":3}`), + client: githubTreeClient{tree: github.Tree{{Name: "doc.md", Checksum: "sum"}}}, + }, + WantScore: mathutil.Hundred, WantStatus: StatusActive, }, "Path deleted since reset scores zero": { @@ -108,7 +133,8 @@ func Test_GithubTracking_Process(t *testing.T) { state: State(`{"pathsChecksums":{"doc.md":"sum"}}`), client: githubTreeClient{tree: github.Tree{}}, }, - WantStatus: StatusActive, + WantStatus: StatusActive, + WantChanged: 1, }, "Path absent since reset scores full": { Inp: githubTreeInput{ @@ -123,7 +149,8 @@ func Test_GithubTracking_Process(t *testing.T) { state: State(`{"pathsChecksums":{}}`), client: githubTreeClient{tree: github.Tree{{Name: "doc.md", Checksum: "sum"}}}, }, - WantStatus: StatusActive, + WantStatus: StatusActive, + WantChanged: 1, }, } @@ -131,10 +158,15 @@ func Test_GithubTracking_Process(t *testing.T) { t.Run(cn, func(t *testing.T) { t.Parallel() + paths := c.Paths + if paths == nil { + paths = []string{"doc.md"} + } + gt := &GithubTracking{ Repository: "repo", Branch: "main", - Paths: []string{"doc.md"}, + Paths: paths, } res, err := gt.Process(context.Background(), c.Inp) @@ -150,7 +182,14 @@ func Test_GithubTracking_Process(t *testing.T) { if c.WantStatus != StatusActive { assert.Nil(t, res.State) + + return } + + var gts GithubTrackingState + + require.NoError(t, json.Unmarshal(res.State, >s)) + assert.Equal(t, c.WantChanged, gts.ChangedPaths) }) } } @@ -169,7 +208,7 @@ func Test_GithubTracking_Reset(t *testing.T) { client: githubTreeClient{tree: github.Tree{{Name: "doc.md", Checksum: "sum"}}}, }, WantStatus: StatusActive, - WantState: `{"pathsChecksums":{"doc.md":"sum"}}`, + WantState: `{"pathsChecksums":{"doc.md":"sum"},"changedPaths":0}`, }, "Missing installation is a status": { Inp: githubErrInput{err: github.ErrInstallationNotFound}, diff --git a/server/core/internal/notification/codes.go b/server/core/internal/notification/codes.go index 835f4222..40b598e9 100644 --- a/server/core/internal/notification/codes.go +++ b/server/core/internal/notification/codes.go @@ -15,15 +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" + MetaKeyUserID = "userId" + MetaKeyDocumentID = "documentId" + MetaKeyBranchID = "branchId" + MetaKeyBlockID = "blockId" + MetaKeyType = "type" + MetaKeyCommentID = "commentId" + MetaKeyCommentReplyID = "commentReplyId" + MetaKeyAnchorBlockID = "anchorBlockId" + MetaKeyStatus = "status" + MetaKeyHookSettings = "hookSettings" + MetaKeyGithubTrackingChangedPaths = "githubTrackingChangedPaths" + MetaKeyCommentExcerpt = "commentExcerpt" ) const ( @@ -42,6 +45,9 @@ const ( // NotificationDocumentNewCommentReply is the notification code for new replies to document comments. NotificationDocumentNewCommentReply Code = "notification.document.new_comment_reply" + + // NotificationDocumentCommentResolved is the notification code for resolved document comments. + NotificationDocumentCommentResolved Code = "notification.document.comment_resolved" ) // NewDocumentReviewRequestNotification creates a new document review request notification core. @@ -56,65 +62,84 @@ func NewDocumentReviewRequestNotification(userID string, documentID, branchID xi } } -// NewDocumentHookTriggeredNotification creates a new document hook triggered notification core. -func NewDocumentHookTriggeredNotification(documentID xid.ID, tp hook.Type, blockID null.String, branchID xid.ID) Core { +// 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. +func NewDocumentHookTriggeredNotification( + documentID xid.ID, + tp hook.Type, + blockID null.String, + branchID xid.ID, + settings processor.Settings, + changedPaths null.Int, +) Core { return Core{ Code: NotificationDocumentHookTriggered, Metadata: map[string]any{ - MetaKeyDocumentID: documentID, - MetaKeyBlockID: blockID, - MetaKeyType: tp, - MetaKeyBranchID: branchID, + MetaKeyDocumentID: documentID, + MetaKeyBlockID: blockID, + MetaKeyType: tp, + MetaKeyBranchID: branchID, + MetaKeyHookSettings: settings, + MetaKeyGithubTrackingChangedPaths: changedPaths, }, } } // NewDocumentHookNeedsAttentionNotification creates a new notification -// core for a document hook that can no longer check its target. +// core for a document hook that can no longer check its target. It keeps +// the settings the hook had at that time. func NewDocumentHookNeedsAttentionNotification( documentID xid.ID, tp hook.Type, blockID null.String, branchID xid.ID, status processor.Status, + settings processor.Settings, ) Core { return Core{ Code: NotificationDocumentHookNeedsAttention, Metadata: map[string]any{ - MetaKeyDocumentID: documentID, - MetaKeyBlockID: blockID, - MetaKeyType: tp, - MetaKeyBranchID: branchID, - MetaKeyStatus: status, + MetaKeyDocumentID: documentID, + MetaKeyBlockID: blockID, + MetaKeyType: tp, + MetaKeyBranchID: branchID, + MetaKeyStatus: status, + MetaKeyHookSettings: settings, }, } } -// NewDocumentNewCommentNotification creates a new document new comment notification core. +// NewDocumentNewCommentNotification creates a new document new comment +// notification core. The excerpt is the start of the comment's text. func NewDocumentNewCommentNotification( userID string, documentID, commentID xid.ID, anchorBlockID null.String, branchID xid.ID, + excerpt string, ) Core { return Core{ Code: NotificationDocumentNewComment, Metadata: map[string]any{ - MetaKeyUserID: userID, - MetaKeyDocumentID: documentID, - MetaKeyCommentID: commentID, - MetaKeyAnchorBlockID: anchorBlockID, - MetaKeyBranchID: branchID, + MetaKeyUserID: userID, + MetaKeyDocumentID: documentID, + MetaKeyCommentID: commentID, + MetaKeyAnchorBlockID: anchorBlockID, + MetaKeyBranchID: branchID, + MetaKeyCommentExcerpt: excerpt, }, } } -// NewDocumentNewCommentReplyNotification creates a new document new comment reply notification core. +// NewDocumentNewCommentReplyNotification creates a new document new comment +// reply notification core. The excerpt is the start of the reply's text. func NewDocumentNewCommentReplyNotification( userID string, documentID, commentID, commentReplyID xid.ID, anchorBlockID null.String, branchID xid.ID, + excerpt string, ) Core { return Core{ Code: NotificationDocumentNewCommentReply, @@ -125,6 +150,30 @@ func NewDocumentNewCommentReplyNotification( MetaKeyCommentReplyID: commentReplyID, MetaKeyAnchorBlockID: anchorBlockID, MetaKeyBranchID: branchID, + MetaKeyCommentExcerpt: excerpt, + }, + } +} + +// NewDocumentCommentResolvedNotification creates a new document comment +// resolved notification core. The user is the one who resolved the comment, +// and the excerpt is the start of the comment's text. +func NewDocumentCommentResolvedNotification( + userID string, + documentID, commentID xid.ID, + anchorBlockID null.String, + branchID xid.ID, + excerpt string, +) Core { + return Core{ + Code: NotificationDocumentCommentResolved, + Metadata: map[string]any{ + MetaKeyUserID: userID, + MetaKeyDocumentID: documentID, + MetaKeyCommentID: commentID, + MetaKeyAnchorBlockID: anchorBlockID, + MetaKeyBranchID: branchID, + MetaKeyCommentExcerpt: excerpt, }, } } diff --git a/server/core/internal/notification/codes_test.go b/server/core/internal/notification/codes_test.go index 688fa26b..238d0ce1 100644 --- a/server/core/internal/notification/codes_test.go +++ b/server/core/internal/notification/codes_test.go @@ -34,18 +34,22 @@ func Test_NewDocumentHookTriggeredNotification(t *testing.T) { nc := NewDocumentHookTriggeredNotification( documentID, - hook.TypeScheduledReminder, + hook.TypeGithubTracking, null.StringFrom("blk1"), branchID, + processor.Settings(`{"repository":"repo"}`), + null.IntFrom(2), ) assert.Equal(t, Core{ Code: NotificationDocumentHookTriggered, Metadata: Metadata{ - MetaKeyDocumentID: documentID, - MetaKeyBlockID: null.StringFrom("blk1"), - MetaKeyType: hook.TypeScheduledReminder, - MetaKeyBranchID: branchID, + MetaKeyDocumentID: documentID, + MetaKeyBlockID: null.StringFrom("blk1"), + MetaKeyType: hook.TypeGithubTracking, + MetaKeyBranchID: branchID, + MetaKeyHookSettings: processor.Settings(`{"repository":"repo"}`), + MetaKeyGithubTrackingChangedPaths: null.IntFrom(2), }, }, nc) } @@ -61,16 +65,18 @@ func Test_NewDocumentHookNeedsAttentionNotification(t *testing.T) { null.StringFrom("blk1"), branchID, processor.StatusMissingRepository, + processor.Settings(`{"repository":"repo"}`), ) assert.Equal(t, Core{ Code: NotificationDocumentHookNeedsAttention, Metadata: Metadata{ - MetaKeyDocumentID: documentID, - MetaKeyBlockID: null.StringFrom("blk1"), - MetaKeyType: hook.TypeGithubTracking, - MetaKeyBranchID: branchID, - MetaKeyStatus: processor.StatusMissingRepository, + MetaKeyDocumentID: documentID, + MetaKeyBlockID: null.StringFrom("blk1"), + MetaKeyType: hook.TypeGithubTracking, + MetaKeyBranchID: branchID, + MetaKeyStatus: processor.StatusMissingRepository, + MetaKeyHookSettings: processor.Settings(`{"repository":"repo"}`), }, }, nc) } @@ -86,16 +92,18 @@ func Test_NewDocumentNewCommentNotification(t *testing.T) { commentID, null.StringFrom("blk1"), branchID, + "Worth a second look", ) assert.Equal(t, Core{ Code: NotificationDocumentNewComment, Metadata: Metadata{ - MetaKeyUserID: "user1", - MetaKeyDocumentID: documentID, - MetaKeyCommentID: commentID, - MetaKeyAnchorBlockID: null.StringFrom("blk1"), - MetaKeyBranchID: branchID, + MetaKeyUserID: "user1", + MetaKeyDocumentID: documentID, + MetaKeyCommentID: commentID, + MetaKeyAnchorBlockID: null.StringFrom("blk1"), + MetaKeyBranchID: branchID, + MetaKeyCommentExcerpt: "Worth a second look", }, }, nc) } @@ -112,6 +120,7 @@ func Test_NewDocumentNewCommentReplyNotification(t *testing.T) { commentReplyID, null.StringFrom("blk1"), branchID, + "Agreed", ) assert.Equal(t, Core{ @@ -123,6 +132,34 @@ func Test_NewDocumentNewCommentReplyNotification(t *testing.T) { MetaKeyCommentReplyID: commentReplyID, MetaKeyAnchorBlockID: null.StringFrom("blk1"), MetaKeyBranchID: branchID, + MetaKeyCommentExcerpt: "Agreed", + }, + }, nc) +} + +func Test_NewDocumentCommentResolvedNotification(t *testing.T) { + t.Parallel() + + documentID, commentID, branchID := xid.New(), xid.New(), xid.New() + + nc := NewDocumentCommentResolvedNotification( + "user1", + documentID, + commentID, + null.StringFrom("blk1"), + branchID, + "Worth a second look", + ) + + assert.Equal(t, Core{ + Code: NotificationDocumentCommentResolved, + Metadata: Metadata{ + MetaKeyUserID: "user1", + MetaKeyDocumentID: documentID, + MetaKeyCommentID: commentID, + MetaKeyAnchorBlockID: null.StringFrom("blk1"), + MetaKeyBranchID: branchID, + MetaKeyCommentExcerpt: "Worth a second look", }, }, nc) } diff --git a/server/core/internal/notification/interpreter/interpreter.go b/server/core/internal/notification/interpreter/interpreter.go index 6e43d2d4..f2244f80 100644 --- a/server/core/internal/notification/interpreter/interpreter.go +++ b/server/core/internal/notification/interpreter/interpreter.go @@ -30,6 +30,10 @@ const ( // message. _commentReplyVerb = "replied to a comment on" + // _commentResolvedVerb names what the resolver did in a comment + // resolved message. + _commentResolvedVerb = "resolved a comment on" + // _hookTriggeredFormat is the hook triggered message, taking the // linked subject and the hook type. _hookTriggeredFormat = "%s may be outdated — %s" @@ -68,6 +72,8 @@ func (i *Interpreter) InterpretNotification(ctx context.Context, n notification. return i.interpretDocumentCommentNotification(ctx, n, _commentVerb) case notification.NotificationDocumentNewCommentReply: return i.interpretDocumentCommentNotification(ctx, n, _commentReplyVerb) + case notification.NotificationDocumentCommentResolved: + return i.interpretDocumentCommentNotification(ctx, n, _commentResolvedVerb) } return nil, ErrUnknownNotificationCode diff --git a/server/core/internal/notification/interpreter/interpreter_test.go b/server/core/internal/notification/interpreter/interpreter_test.go index 33a15b3a..9b6018c1 100644 --- a/server/core/internal/notification/interpreter/interpreter_test.go +++ b/server/core/internal/notification/interpreter/interpreter_test.go @@ -103,6 +103,8 @@ func Test_Interpreter_InterpretNotification(t *testing.T) { hook.TypeScheduledReminder, null.String{}, _testBranchID, + nil, + null.Int{}, )), Result: &Message{ Text: fmt.Sprintf("<%s|My Doc> may be outdated — Scheduled Reminder", _testDocURL), @@ -115,6 +117,7 @@ func Test_Interpreter_InterpretNotification(t *testing.T) { null.String{}, _testBranchID, processor.StatusMissingRepository, + nil, )), Result: &Message{ Text: fmt.Sprintf("<%s|My Doc> can no longer be checked — GitHub Tracking", _testDocURL), @@ -127,6 +130,7 @@ func Test_Interpreter_InterpretNotification(t *testing.T) { xid.New(), null.String{}, _testBranchID, + "", )), Result: &Message{ Text: fmt.Sprintf("Alice left a comment on <%s|My Doc>", _testDocURL), @@ -140,11 +144,25 @@ func Test_Interpreter_InterpretNotification(t *testing.T) { xid.New(), null.String{}, _testBranchID, + "", )), Result: &Message{ Text: fmt.Sprintf("Alice replied to a comment on <%s|My Doc>", _testDocURL), }, }, + "Document comment resolved": { + N: stubNotification(notification.NewDocumentCommentResolvedNotification( + "user1", + _testDocID, + xid.New(), + null.String{}, + _testBranchID, + "", + )), + Result: &Message{ + Text: fmt.Sprintf("Alice resolved a comment on <%s|My Doc>", _testDocURL), + }, + }, } for cn, c := range cc { @@ -303,6 +321,8 @@ func Test_Interpreter_interpretDocumentHookNotification(t *testing.T) { hook.TypeScheduledReminder, null.String{}, _testBranchID, + nil, + null.Int{}, )), Err: assert.AnError, }, @@ -320,6 +340,8 @@ func Test_Interpreter_interpretDocumentHookNotification(t *testing.T) { hook.TypeScheduledReminder, null.String{}, _testBranchID, + nil, + null.Int{}, )), Err: assert.AnError, }, @@ -330,6 +352,8 @@ func Test_Interpreter_interpretDocumentHookNotification(t *testing.T) { hook.TypeScheduledReminder, null.String{}, _testBranchID, + nil, + null.Int{}, )), Result: &Message{ Text: fmt.Sprintf("<%s|My Doc> may be outdated — Scheduled Reminder", _testDocURL), @@ -359,6 +383,8 @@ func Test_Interpreter_interpretDocumentHookNotification(t *testing.T) { hook.TypeScheduledReminder, null.StringFrom("blk1"), _testBranchID, + nil, + null.Int{}, )), Result: &Message{ Text: fmt.Sprintf("<%s#blk1|a block in My Doc> may be outdated — Scheduled Reminder", _testDocURL), @@ -372,6 +398,7 @@ func Test_Interpreter_interpretDocumentHookNotification(t *testing.T) { null.StringFrom("blk1"), _testBranchID, processor.StatusUnreachableURL, + nil, )), Format: _hookNeedsAttentionFormat, Result: &Message{ @@ -459,6 +486,7 @@ func Test_Interpreter_interpretDocumentCommentNotification(t *testing.T) { xid.New(), null.String{}, _testBranchID, + "", )), Verb: _commentVerb, Err: assert.AnError, @@ -478,6 +506,7 @@ func Test_Interpreter_interpretDocumentCommentNotification(t *testing.T) { xid.New(), null.String{}, _testBranchID, + "", )), Verb: _commentVerb, Err: assert.AnError, @@ -497,6 +526,7 @@ func Test_Interpreter_interpretDocumentCommentNotification(t *testing.T) { xid.New(), null.String{}, _testBranchID, + "", )), Verb: _commentVerb, Err: assert.AnError, @@ -509,6 +539,7 @@ func Test_Interpreter_interpretDocumentCommentNotification(t *testing.T) { xid.New(), null.String{}, _testBranchID, + "", )), Verb: _commentVerb, Result: &Message{ @@ -523,6 +554,7 @@ func Test_Interpreter_interpretDocumentCommentNotification(t *testing.T) { xid.New(), null.StringFrom("blk1"), _testBranchID, + "", )), Verb: _commentVerb, Result: &Message{ @@ -538,6 +570,7 @@ func Test_Interpreter_interpretDocumentCommentNotification(t *testing.T) { xid.New(), null.String{}, _testBranchID, + "", )), Verb: _commentReplyVerb, Result: &Message{ @@ -553,12 +586,28 @@ func Test_Interpreter_interpretDocumentCommentNotification(t *testing.T) { xid.New(), null.StringFrom("blk1"), _testBranchID, + "", )), Verb: _commentReplyVerb, Result: &Message{ Text: fmt.Sprintf("Alice replied to a comment on <%s#blk1|My Doc>", _testDocURL), }, }, + "Successful resolved interpretation": { + DB: stubDB(), + N: stubNotification(notification.NewDocumentCommentResolvedNotification( + "user1", + _testDocID, + xid.New(), + null.String{}, + _testBranchID, + "", + )), + Verb: _commentResolvedVerb, + Result: &Message{ + Text: fmt.Sprintf("Alice resolved a comment on <%s|My Doc>", _testDocURL), + }, + }, } for cn, c := range cc { diff --git a/server/core/internal/notification/notification_test.go b/server/core/internal/notification/notification_test.go index 8b12da5e..282a9782 100644 --- a/server/core/internal/notification/notification_test.go +++ b/server/core/internal/notification/notification_test.go @@ -5,6 +5,7 @@ import ( "testing" "time" + "github.com/oxynote/oxynote/server/core/internal/document/hook/processor" "github.com/oxynote/oxynote/server/core/pkg/testutil" "github.com/oxynote/oxynote/server/core/pkg/timeutil" "github.com/stretchr/testify/assert" @@ -33,12 +34,20 @@ func Test_newNotification(t *testing.T) { func Test_Metadata_Value(t *testing.T) { t.Parallel() - md := Metadata{MetaKeyUserID: "user1", "read": true} + md := Metadata{ + MetaKeyUserID: "user1", + "read": true, + MetaKeyHookSettings: processor.Settings(`{"url":"https://example.com"}`), + } v, err := md.Value() require.NoError(t, err) - assert.JSONEq(t, `{"userId":"user1","read":true}`, string(v.([]byte))) + assert.JSONEq( + t, + `{"userId":"user1","read":true,"hookSettings":{"url":"https://example.com"}}`, + string(v.([]byte)), + ) } func Test_Metadata_Scan(t *testing.T) { diff --git a/server/core/internal/server/internal/document/comment/http.go b/server/core/internal/server/internal/document/comment/http.go index 386be892..ac50d2ee 100644 --- a/server/core/internal/server/internal/document/comment/http.go +++ b/server/core/internal/server/internal/document/comment/http.go @@ -134,6 +134,7 @@ func (h *Handler) CreateDocumentComment(w http.ResponseWriter, r *http.Request) c.ID, c.AnchorBlockID, c.BranchID, + c.Content.Excerpt(), ), userIDs..., ) @@ -217,6 +218,7 @@ func (h *Handler) CreateDocumentCommentReply(w http.ResponseWriter, r *http.Requ reply.ID, c.AnchorBlockID, c.BranchID, + reply.Content.Excerpt(), ), commentCore.Recipients(c.Participants(), session.UserID)..., ) @@ -537,6 +539,21 @@ func (h *Handler) UpdateDocumentCommentStatus(w http.ResponseWriter, r *http.Req }) } + if nc.Resolved { + h.notifPub.PublishNotifications( + session.ActiveOrganizationID, + notification.NewDocumentCommentResolvedNotification( + session.UserID, + documentID, + nc.ID, + nc.AnchorBlockID, + nc.BranchID, + nc.Content.Excerpt(), + ), + commentCore.Recipients(nc.Participants(), session.UserID)..., + ) + } + httpserver.Respond( h.log, w, diff --git a/server/core/internal/server/internal/document/comment/http_test.go b/server/core/internal/server/internal/document/comment/http_test.go index 7c80ff7a..2d8f3219 100644 --- a/server/core/internal/server/internal/document/comment/http_test.go +++ b/server/core/internal/server/internal/document/comment/http_test.go @@ -2,6 +2,7 @@ package comment import ( "context" + "encoding/json" "errors" "log/slog" "net/http" @@ -121,6 +122,9 @@ func newRequest(method, body string, noSession, omitDoc, omitComment, omitReply return req.WithContext(ctx) } +// _docContent is comment content in the editor's shape, reading "hello". +const _docContent = `{"type":"doc","content":[{"type":"paragraph","content":[{"type":"text","text":"hello"}]}]}` + // storedComment builds a stored comment owned by the given user. func storedComment(userID string, replies ...commentCore.Reply) *commentCore.Comment { return &commentCore.Comment{ @@ -158,7 +162,7 @@ func Test_NewHandler(t *testing.T) { } func Test_Handler_CreateDocumentComment(t *testing.T) { - validBody := `{"content":{"text":"hello"},"branchId":"` + _branchID.String() + `"}` + validBody := `{"content":` + _docContent + `,"branchId":"` + _branchID.String() + `"}` cc := map[string]struct { DB *DBMock @@ -282,13 +286,15 @@ func Test_Handler_CreateDocumentComment(t *testing.T) { for i, users := range c.NotifyUsers { assert.Equal(t, "org1", pub.calls[i].OrganizationID) assert.Equal(t, users, pub.calls[i].UserIDs) + assert.Equal(t, notification.NotificationDocumentNewComment, pub.calls[i].Core.Code) + assert.Equal(t, "hello", pub.calls[i].Core.Metadata[notification.MetaKeyCommentExcerpt]) } }) } } func Test_Handler_CreateDocumentCommentReply(t *testing.T) { - validBody := `{"content":{"text":"reply"}}` + validBody := `{"content":` + _docContent + `}` cc := map[string]struct { DB *DBMock @@ -398,6 +404,8 @@ func Test_Handler_CreateDocumentCommentReply(t *testing.T) { for i, users := range c.NotifyUsers { assert.Equal(t, users, pub.calls[i].UserIDs) + assert.Equal(t, notification.NotificationDocumentNewCommentReply, pub.calls[i].Core.Code) + assert.Equal(t, "hello", pub.calls[i].Core.Metadata[notification.MetaKeyCommentExcerpt]) } }) } @@ -836,14 +844,15 @@ func Test_Handler_UpdateDocumentCommentStatus(t *testing.T) { } cc := map[string]struct { - DB *DBMock - NoSession bool - OmitDoc bool - Body string - RespCode int - Updated int - Changes int - Check func(t *testing.T, c commentCore.Comment) + DB *DBMock + NoSession bool + OmitDoc bool + Body string + RespCode int + Updated int + Changes int + NotifyUsers [][]string + Check func(t *testing.T, c commentCore.Comment) }{ "No session in context": { DB: &DBMock{}, @@ -897,16 +906,23 @@ func Test_Handler_UpdateDocumentCommentStatus(t *testing.T) { RespCode: http.StatusInternalServerError, Updated: 1, }, - "Successful resolution of another user's comment": { + "Successful resolution notifies author and repliers": { DB: &DBMock{ FetchDocumentCommentFunc: func(context.Context, xid.ID, xid.ID, string) (*commentCore.Comment, error) { - return storedComment("u2"), nil + c := storedComment("u2", storedReply("u1"), storedReply("u3")) + + if err := json.Unmarshal([]byte(_docContent), &c.Content); err != nil { + return nil, err + } + + return c, nil }, }, - Body: `{"resolved":true}`, - RespCode: http.StatusOK, - Updated: 1, - Changes: 1, + Body: `{"resolved":true}`, + RespCode: http.StatusOK, + Updated: 1, + Changes: 1, + NotifyUsers: [][]string{{"u2", "u3"}}, Check: func(t *testing.T, c commentCore.Comment) { assert.True(t, c.Resolved) assert.Equal(t, null.StringFrom("u1"), c.ResolvedBy) @@ -962,7 +978,15 @@ func Test_Handler_UpdateDocumentCommentStatus(t *testing.T) { hdl.UpdateDocumentCommentStatus(rec, newRequest(http.MethodPut, c.Body, c.NoSession, c.OmitDoc, false, true)) assert.Equal(t, c.RespCode, rec.Code) - assert.Empty(t, pub.calls) + require.Len(t, pub.calls, len(c.NotifyUsers)) + + for i, users := range c.NotifyUsers { + assert.Equal(t, "org1", pub.calls[i].OrganizationID) + assert.Equal(t, users, pub.calls[i].UserIDs) + assert.Equal(t, notification.NotificationDocumentCommentResolved, pub.calls[i].Core.Code) + assert.Equal(t, "u1", pub.calls[i].Core.Metadata[notification.MetaKeyUserID]) + assert.Equal(t, "hello", pub.calls[i].Core.Metadata[notification.MetaKeyCommentExcerpt]) + } uu := c.DB.UpdateDocumentCommentCalls() require.Len(t, uu, c.Updated) diff --git a/web/app/components/NotificationBox.nuxt.test.ts b/web/app/components/NotificationBox.nuxt.test.ts index 277654a7..95eb9e9d 100644 --- a/web/app/components/NotificationBox.nuxt.test.ts +++ b/web/app/components/NotificationBox.nuxt.test.ts @@ -1,11 +1,14 @@ -import { mockNuxtImport } from "@nuxt/test-utils/runtime" -import { flushPromises } from "@vue/test-utils" +import { mockNuxtImport, mountSuspended } from "@nuxt/test-utils/runtime" +import { setInfiniteQueryData } from "@pinia/colada" +import { enableAutoUnmount, flushPromises } from "@vue/test-utils" import { afterEach, beforeEach, describe, it, vi } from "vitest" import { toast } from "vue-sonner" import { clearQueryCache, disposeMockEndpoints, + mockDeferredEndpoint, mockEndpoint, + runInApp, seedQueryData, } from "~/composables/api/test-helpers" import NotificationBox from "./NotificationBox.vue" @@ -13,8 +16,6 @@ import { at, findButtonByText, mockAuthOrganization, - mountUnderTooltipProvider, - openTooltipText, renderedIconNames, seedAuthOrganization, t, @@ -29,62 +30,46 @@ vi.mock("vue-sonner", () => ({ const navigateToMock = vi.hoisted(() => vi.fn()) mockNuxtImport("navigateTo", () => navigateToMock) -const TOOLTIP_DELAY_MS = 600 +// happy-dom lays nothing out, so the real scroll watcher would see the list +// as always at its end. The box's callbacks are driven by hand instead. +const infiniteScroll = vi.hoisted(() => ({ + load: (): unknown => undefined, + canLoadMore: (): boolean => false, +})) +mockNuxtImport( + "useInfiniteScroll", + () => + ( + _element: unknown, + onLoadMore: () => unknown, + options: { canLoadMore: () => boolean }, + ) => { + infiniteScroll.load = onLoadMore + infiniteScroll.canLoadMore = options.canLoadMore + }, +) + const DOC_ID = "doc1".padEnd(20, "0") const USER_ID = "user".padEnd(20, "0") - -function makeNotification(overrides: Record = {}) { - return { - id: "notif-1", - userId: USER_ID, - organizationId: "org-1", - code: NotificationCode.DocumentReviewRequest, - metadata: { documentId: DOC_ID, branchId: "branch-1" }, - read: false, - createdAt: "2026-03-14T11:00:00Z", - ...overrides, - } -} - -function seedNotifications(notifications: unknown[]) { - seedQueryData(["notifications", "list", 100, 1], { - notifications: notifications, - pageCount: 1, - }) - seedQueryData(["notifications", "count", false], { count: 0 }) -} - -function seedTree() { - seedQueryData( - ["documents", "tree"], - [ - { - id: DOC_ID, - documentName: "Runbook", - icon: "lucide:file", - protected: false, - children: null, - }, - ], - ) -} - -function mountBox(props: Record = {}) { - return mountUnderTooltipProvider(NotificationBox, { props: props }) -} - -function rows(wrapper: Awaited>) { - return wrapper.findAll("div.cursor-pointer") +const COMMENT_METADATA = { + userId: USER_ID, + documentId: DOC_ID, + branchId: "b", + commentId: "c", + anchorBlockId: null, } // the query cache, the pinia websocket store and the vue-sonner module // mock are all app-wide singletons every mount in the file shares describe("", { concurrent: false }, () => { + // the box keeps a clock and a polling query running while mounted + enableAutoUnmount(afterEach) + beforeEach(() => { clearQueryCache() vi.mocked(toast.custom).mockReset() navigateToMock.mockReset() - vi.setSystemTime(new Date("2026-03-14T12:00:00Z")) + vi.setSystemTime(new Date(2026, 2, 14, 12, 0, 0)) // a successful mark-read invalidates both notification queries; without // these the refetch rejects and drags the mutation down with it, so the // failure toast lands in whichever test is running by then @@ -118,7 +103,56 @@ describe("", { concurrent: false }, () => { expect(rows(wrapper)).toHaveLength(2) }) - it("names the document the notification is about", async ({ expect }) => { + it("groups the notifications under the day they arrived", async ({ + expect, + }) => { + seedNotifications([ + makeNotification({ id: "n1", createdAt: new Date(2026, 2, 14, 9) }), + makeNotification({ id: "n2", createdAt: new Date(2026, 2, 13, 9) }), + makeNotification({ id: "n3", createdAt: new Date(2026, 2, 13, 8) }), + makeNotification({ id: "n4", createdAt: new Date(2026, 2, 2, 9) }), + ]) + + const wrapper = await mountBox() + + expect( + wrapper.findAll("section").map((section) => ({ + day: section.get("h3").text(), + rows: section.findAll("[role='link']").length, + })), + ).toEqual([ + { day: t("notification.days.today"), rows: 1 }, + { day: t("notification.days.yesterday"), rows: 2 }, + { day: t("notification.days.earlier"), rows: 1 }, + ]) + }) + + it("leaves out a day without notifications", async ({ expect }) => { + seedNotifications([ + makeNotification({ createdAt: new Date(2026, 2, 2, 9) }), + ]) + + const wrapper = await mountBox() + + expect(wrapper.findAll("h3").map((heading) => heading.text())).toEqual([ + t("notification.days.earlier"), + ]) + }) + + it("shows a notification the next page repeats only once", async ({ + expect, + }) => { + seedPages([ + [makeNotification({ id: "n1" }), makeNotification({ id: "n2" })], + [makeNotification({ id: "n2" }), makeNotification({ id: "n3" })], + ]) + + const wrapper = await mountBox() + + expect(rows(wrapper)).toHaveLength(3) + }) + + it("names the page the notification is about", async ({ expect }) => { seedTree() seedNotifications([makeNotification()]) @@ -127,311 +161,130 @@ describe("", { concurrent: false }, () => { expect(wrapper.text()).toContain("Runbook") }) - it("spells out the document name in a tooltip once hovered", async ({ - expect, - }) => { - vi.useFakeTimers() - seedTree() - seedNotifications([makeNotification()]) - - const wrapper = await mountBox() - await at(wrapper.findAll("[data-slot='tooltip-trigger']"), 0).trigger( - "pointermove", - ) - await vi.advanceTimersByTimeAsync(TOOLTIP_DELAY_MS) - - expect(openTooltipText(wrapper)).toBe("Runbook") - }) - - it("falls back to a placeholder name for a deleted document", async ({ - expect, - }) => { - seedNotifications([makeNotification()]) - - const wrapper = await mountBox() - - expect(wrapper.text()).toContain(t("notification.document-fallback")) - }) - - it("labels a notification from the last minute as new", async ({ - expect, - }) => { - seedNotifications([makeNotification({ createdAt: "2026-03-14T11:59:30Z" })]) - - const wrapper = await mountBox() - - expect(wrapper.text()).toContain(t("notification.now-time-label")) - }) - - it("shows how long ago an older notification arrived", async ({ expect }) => { - seedNotifications([makeNotification({ createdAt: "2026-03-14T11:00:00Z" })]) - - const wrapper = await mountBox() - - expect(wrapper.text()).toContain("1 hour ago") - }) - - it("dims a notification that has been read", async ({ expect }) => { - seedNotifications([makeNotification({ read: true })]) - - const wrapper = await mountBox() - - expect(rows(wrapper)[0]?.classes()).toContain("opacity-60") - }) - - it("offers a mark-read button only on unread notifications", async ({ - expect, - }) => { + it("names the member who commented", async ({ expect }) => { + seedAuthOrganization({ + members: [{ userId: USER_ID, user: { name: "Ada" } }], + }) seedNotifications([ - makeNotification({ id: "notif-1", read: false }), - makeNotification({ id: "notif-2", read: true }), + makeNotification({ + code: NotificationCode.DocumentNewComment, + metadata: COMMENT_METADATA, + }), ]) const wrapper = await mountBox() - expect(wrapper.text().match(/Mark as read/g)).toHaveLength(1) + expect(wrapper.text()).toContain( + t("notification.messages.document-new-comment-description", { + user: "Ada", + }), + ) }) - describe("descriptions", { concurrent: false }, () => { - it("describes a review request", async ({ expect }) => { - seedNotifications([makeNotification()]) + it("calls a commenter who left the organization a deleted user", async ({ + expect, + }) => { + seedAuthOrganization({ members: [] }) + seedNotifications([ + makeNotification({ + code: NotificationCode.DocumentNewComment, + metadata: COMMENT_METADATA, + }), + ]) - const wrapper = await mountBox() + const wrapper = await mountBox() - expect(wrapper.text()).toContain( - t("notification.messages.document-review-request-description"), - ) - }) - - it.for([ - { - name: "names the hook that fired", - input: { - code: NotificationCode.DocumentHookTrigerred, - type: DocumentHookType.URLWatcher, - }, - expected: "notification.messages.document-hook-triggered-description", - }, - { - name: "names the hook that needs attention", - input: { - code: NotificationCode.DocumentHookNeedsAttention, - type: DocumentHookType.GitHubTracking, - }, - expected: - "notification.messages.document-hook-needs-attention-description", - }, - ])("$name", async ({ input, expected }, { expect }) => { - seedNotifications([ - makeNotification({ - code: input.code, - metadata: { - documentId: DOC_ID, - branchId: "b", - blockId: null, - type: input.type, - }, - }), - ]) - - const wrapper = await mountBox() - - expect(wrapper.text()).toContain( - t(expected, { hook: t(`editor.hooks.${input.type}.title`) }), - ) - }) - - it("names the commenter on a new comment", async ({ expect }) => { - seedAuthOrganization({ - members: [{ userId: USER_ID, user: { name: "Ada" } }], - }) - seedNotifications([ - makeNotification({ - code: NotificationCode.DocumentNewComment, - metadata: { - userId: USER_ID, - documentId: DOC_ID, - branchId: "b", - commentId: "c", - anchorBlockId: null, - }, - }), - ]) - - const wrapper = await mountBox() - - expect(wrapper.text()).toContain("Ada has posted a new comment") - }) - - it("names the commenter on a reply", async ({ expect }) => { - seedAuthOrganization({ - members: [{ userId: USER_ID, user: { name: "Ada" } }], - }) - seedNotifications([ - makeNotification({ - code: NotificationCode.DocumentNewCommentReply, - metadata: { - userId: USER_ID, - documentId: DOC_ID, - branchId: "b", - commentId: "c", - commentReplyId: "r", - anchorBlockId: null, - }, - }), - ]) - - const wrapper = await mountBox() - - expect(wrapper.text()).toContain("Ada has posted a reply to a comment") - }) - - it("calls an unknown commenter a deleted user", async ({ expect }) => { - seedAuthOrganization({ members: [] }) - seedNotifications([ - makeNotification({ - code: NotificationCode.DocumentNewComment, - metadata: { - userId: USER_ID, - documentId: DOC_ID, - branchId: "b", - commentId: "c", - anchorBlockId: null, - }, - }), - ]) - - const wrapper = await mountBox() - - expect(wrapper.text()).toContain( - t("notification.messages.document-new-comment-description", { - user: t("general.deleted-user"), - }), - ) - }) - - it("falls back to a generic description for an unknown code", async ({ - expect, - }) => { - seedNotifications([makeNotification({ code: "notification.unknown" })]) - - const wrapper = await mountBox() - - expect(wrapper.text()).toContain( - t("notification.messages.default-description"), - ) - }) + expect(wrapper.text()).toContain( + t("notification.messages.document-new-comment-description", { + user: t("general.deleted-user"), + }), + ) }) - describe("icons", { concurrent: false }, () => { - it("marks a review request with a reviewer icon", async ({ expect }) => { - seedNotifications([makeNotification()]) + it("closes the box when its collapse button is pressed", async ({ + expect, + }) => { + seedNotifications([]) + const wrapper = await mountBox() - const wrapper = await mountBox() + await findButtonByText( + wrapper, + t("notification.actions.close-notification-box"), + ).trigger("click") - expect(renderedIconNames(wrapper)).toContain("lucide:file-user") - }) + expect(wrapper.emitted("close-notification-box")).toHaveLength(1) + }) + describe("loading more", { concurrent: false }, () => { it.for([ - { - name: "marks a url watcher trigger with a globe icon", - input: { - code: NotificationCode.DocumentHookTrigerred, - type: DocumentHookType.URLWatcher, - }, - expected: "mingcute:earth-2-line", - }, - { - name: "marks a github tracking trigger with a github icon", - input: { - code: NotificationCode.DocumentHookTrigerred, - type: DocumentHookType.GitHubTracking, - }, - expected: "simple-icons:github", - }, - { - name: "marks a scheduled reminder trigger with a timer icon", - input: { - code: NotificationCode.DocumentHookTrigerred, - type: DocumentHookType.ScheduledReminder, - }, - expected: "lucide:timer", - }, - { - name: "marks a container image watcher trigger with a container icon", - input: { - code: NotificationCode.DocumentHookTrigerred, - type: DocumentHookType.ContainerImageWatcher, - }, - expected: "lucide:container", - }, - { - name: "marks a hook that needs attention with its own icon", - input: { - code: NotificationCode.DocumentHookNeedsAttention, - type: DocumentHookType.ContainerImageWatcher, - }, - expected: "lucide:container", - }, - ])("$name", async ({ input, expected }, { expect }) => { - seedNotifications([ - makeNotification({ - code: input.code, - metadata: { - documentId: DOC_ID, - branchId: "b", - blockId: null, - type: input.type, - }, - }), - ]) + { name: "offers more while the server has pages", input: 2 }, + { name: "offers no more on the last page", input: 1 }, + ])("$name", async ({ input }, { expect }) => { + seedPages([[makeNotification()]], input) - const wrapper = await mountBox() + await mountBox() - expect(renderedIconNames(wrapper)).toContain(expected) + expect(infiniteScroll.canLoadMore()).toBe(input > 1) }) - it("marks a hook type this build does not know with a warning icon", async ({ + it("adds the next page when the list nears its end", async ({ expect }) => { + disposeMockEndpoints() + const listCalls = mockEndpoint("GET", "/api/notifications", () => ({ + notifications: [makeNotification({ id: "notif-2" })], + pageCount: 2, + })) + seedPages([[makeNotification()]], 2) + const wrapper = await mountBox() + + await infiniteScroll.load() + await flushPromises() + + expect(rows(wrapper)).toHaveLength(2) + expect(listCalls).toHaveLength(1) + expect(listCalls[0]?.query).toEqual({ limit: "50", page: "2" }) + }) + + it("stops asking for more once a page fails to load", async ({ expect, }) => { - seedNotifications([ - makeNotification({ - code: NotificationCode.DocumentHookTrigerred, - metadata: { - documentId: DOC_ID, - branchId: "b", - blockId: null, - type: "future-hook" as DocumentHookType, - }, - }), - ]) - + disposeMockEndpoints() + // the client retries a GET once on a 5xx, which a 400 keeps out + // of the count + const listCalls = mockEndpoint("GET", "/api/notifications", () => { + throw createError({ statusCode: 400 }) + }) + seedPages([[makeNotification()]], 2) const wrapper = await mountBox() + await infiniteScroll.load() + await flushPromises() + + expect(infiniteScroll.canLoadMore()).toBe(false) + expect(rows(wrapper)).toHaveLength(1) + expect(listCalls).toHaveLength(1) + }) + + it("shows a spinner while the next page loads", async ({ expect }) => { + disposeMockEndpoints() + const list = mockDeferredEndpoint("GET", "/api/notifications") + seedPages([[makeNotification()]], 2) + const wrapper = await mountBox() + + const loading = infiniteScroll.load() + await list.reached + await nextTick() + expect(renderedIconNames(wrapper)).toContain( - "lucide:file-exclamation-point", + "svg-spinners:blocks-shuffle-3", ) - }) - it("marks a comment notification with a message icon", async ({ - expect, - }) => { - seedNotifications([ - makeNotification({ - code: NotificationCode.DocumentNewComment, - metadata: { - userId: USER_ID, - documentId: DOC_ID, - branchId: "b", - commentId: "c", - anchorBlockId: null, - }, - }), - ]) - - const wrapper = await mountBox() - - expect(renderedIconNames(wrapper)).toContain("mingcute:message-4-fill") + // settle the held request so nothing stays in flight + list.resolve({ notifications: [], pageCount: 2 }) + await loading + await flushPromises() + expect(renderedIconNames(wrapper)).not.toContain( + "svg-spinners:blocks-shuffle-3", + ) }) }) @@ -481,7 +334,7 @@ describe("", { concurrent: false }, () => { "/api/notifications/read-status", () => ({}), ) - seedNotifications([makeNotification()]) + seedNotifications([makeNotification()], 1) const wrapper = await mountBox() await findButtonByText( @@ -494,10 +347,20 @@ describe("", { concurrent: false }, () => { expect(calls[0]?.body).toEqual({ ids: [] }) }) - it("hides the button when every notification is already read", async ({ + it("offers to mark everything read while a page not loaded yet holds unread ones", async ({ expect, }) => { - seedNotifications([makeNotification({ read: true })]) + seedNotifications([makeNotification({ read: true })], 3) + + const wrapper = await mountBox() + + expect( + findButtonByText(wrapper, t("notification.read-all-button")).exists(), + ).toBe(true) + }) + + it("hides the button when nothing is unread", async ({ expect }) => { + seedNotifications([makeNotification({ read: true })], 0) const wrapper = await mountBox() @@ -512,7 +375,7 @@ describe("", { concurrent: false }, () => { mockEndpoint("PUT", "/api/notifications/read-status", () => { throw createError({ statusCode: 500 }) }) - seedNotifications([makeNotification()]) + seedNotifications([makeNotification()], 1) const wrapper = await mountBox() await findButtonByText( @@ -531,14 +394,14 @@ describe("", { concurrent: false }, () => { name: "opens a review request at the document", notification: { code: NotificationCode.DocumentReviewRequest, - metadata: { documentId: DOC_ID, branchId: "b" }, + metadata: { userId: USER_ID, documentId: DOC_ID, branchId: "b" }, }, expected: `/acme/Runbook-${DOC_ID}?branch=b`, }, { - name: "opens a hook trigger at the block that fired", + name: "opens a triggered hook at its block", notification: { - code: NotificationCode.DocumentHookTrigerred, + code: NotificationCode.DocumentHookTriggered, metadata: { documentId: DOC_ID, branchId: "b", @@ -549,9 +412,9 @@ describe("", { concurrent: false }, () => { expected: `/acme/Runbook-${DOC_ID}?branch=b#block-7`, }, { - name: "opens a document-wide hook trigger at the document", + name: "opens a triggered page-wide hook at the document", notification: { - code: NotificationCode.DocumentHookTrigerred, + code: NotificationCode.DocumentHookTriggered, metadata: { documentId: DOC_ID, branchId: "b", @@ -578,13 +441,7 @@ describe("", { concurrent: false }, () => { name: "opens a new comment at the block it is anchored to", notification: { code: NotificationCode.DocumentNewComment, - metadata: { - userId: USER_ID, - documentId: DOC_ID, - branchId: "b", - commentId: "c", - anchorBlockId: "block-3", - }, + metadata: { ...COMMENT_METADATA, anchorBlockId: "block-3" }, }, expected: `/acme/Runbook-${DOC_ID}?branch=b#block-3`, }, @@ -592,13 +449,7 @@ describe("", { concurrent: false }, () => { name: "opens an unanchored comment at the document", notification: { code: NotificationCode.DocumentNewComment, - metadata: { - userId: USER_ID, - documentId: DOC_ID, - branchId: "b", - commentId: "c", - anchorBlockId: null, - }, + metadata: COMMENT_METADATA, }, expected: `/acme/Runbook-${DOC_ID}?branch=b`, }, @@ -607,10 +458,7 @@ describe("", { concurrent: false }, () => { notification: { code: NotificationCode.DocumentNewCommentReply, metadata: { - userId: USER_ID, - documentId: DOC_ID, - branchId: "b", - commentId: "c", + ...COMMENT_METADATA, commentReplyId: "r", anchorBlockId: "block-4", }, @@ -621,19 +469,24 @@ describe("", { concurrent: false }, () => { name: "opens an unanchored comment reply at the document", notification: { code: NotificationCode.DocumentNewCommentReply, - metadata: { - userId: USER_ID, - documentId: DOC_ID, - branchId: "b", - commentId: "c", - commentReplyId: "r", - anchorBlockId: null, - }, + metadata: { ...COMMENT_METADATA, commentReplyId: "r" }, }, expected: `/acme/Runbook-${DOC_ID}?branch=b`, }, + { + name: "opens a resolved comment at the block it is anchored to", + notification: { + code: NotificationCode.DocumentCommentResolved, + metadata: { ...COMMENT_METADATA, anchorBlockId: "block-5" }, + }, + expected: `/acme/Runbook-${DOC_ID}?branch=b#block-5`, + }, ])("$name", async ({ notification, expected }, { expect }) => { - mockEndpoint("PUT", "/api/notifications/read-status", () => ({})) + const calls = mockEndpoint( + "PUT", + "/api/notifications/read-status", + () => ({}), + ) mockAuthOrganization({ id: "org-1", slug: "acme", members: [] }) seedTree() seedNotifications([makeNotification(notification)]) @@ -643,6 +496,7 @@ describe("", { concurrent: false }, () => { await flushPromises() expect(navigateToMock).toHaveBeenCalledExactlyOnceWith(expected) + expect(calls.map((call) => call.body)).toEqual([{ ids: ["notif-1"] }]) }) it("goes nowhere for a notification code it does not know", async ({ @@ -662,7 +516,7 @@ describe("", { concurrent: false }, () => { it("stays put when the notification's document no longer exists", async ({ expect, }) => { - seedAuthOrganization({ slug: "acme" }) + seedAuthOrganization({ slug: "acme", members: [] }) seedNotifications([makeNotification()]) const wrapper = await mountBox() @@ -673,36 +527,6 @@ describe("", { concurrent: false }, () => { }) }) - describe("mobile", { concurrent: false }, () => { - it("hides the close button on wide viewports", async ({ expect }) => { - seedNotifications([]) - - const wrapper = await mountBox() - - expect(wrapper.text()).not.toContain( - t("notification.actions.close-notification-box"), - ) - }) - - it("closes the box when its close button is pressed", async ({ - expect, - }) => { - seedNotifications([]) - const wrapper = await mountBox({ mobile: true }) - - await findButtonByText( - wrapper, - t("notification.actions.close-notification-box"), - ).trigger("click") - - expect( - wrapper - .findComponent(NotificationBox) - .emitted("close-notification-box"), - ).toHaveLength(1) - }) - }) - describe("websocket updates", { concurrent: false }, () => { it("subscribes to notification creations while mounted", async ({ expect, @@ -733,3 +557,59 @@ describe("", { concurrent: false }, () => { }) }) }) + +function makeNotification(overrides: Record = {}) { + return { + id: "notif-1", + userId: USER_ID, + organizationId: "org-1", + code: NotificationCode.DocumentReviewRequest, + metadata: { userId: USER_ID, documentId: DOC_ID, branchId: "branch-1" }, + read: false, + createdAt: new Date(2026, 2, 14, 11, 0, 0), + ...overrides, + } +} + +// seeds the pages the box has loaded so far, out of pageCount on the server +function seedPages(pages: unknown[][], pageCount = pages.length, unread = 0) { + // a plain seed leaves the entry without its paging state, and the + // box's query cannot mount on it + runInApp(() => { + setInfiniteQueryData(useQueryCache(), ["notifications", "list", 50], { + pages: pages.map((notifications) => ({ + notifications: notifications, + pageCount: pageCount, + })), + pageParams: pages.map((_page, index) => index + 1), + }) + }) + seedQueryData(["notifications", "count", false], { count: unread }) +} + +function seedNotifications(notifications: unknown[], unread = 0) { + seedPages([notifications], 1, unread) +} + +function seedTree() { + seedQueryData( + ["documents", "tree"], + [ + { + id: DOC_ID, + documentName: "Runbook", + icon: "mingcute:book-2-line", + protected: false, + children: null, + }, + ], + ) +} + +function mountBox() { + return mountSuspended(NotificationBox) +} + +function rows(wrapper: Awaited>) { + return wrapper.findAll("[role='link']") +} diff --git a/web/app/components/NotificationBox.vue b/web/app/components/NotificationBox.vue index 1c460768..d838651e 100644 --- a/web/app/components/NotificationBox.vue +++ b/web/app/components/NotificationBox.vue @@ -1,17 +1,18 @@ + + diff --git a/web/app/components/NotificationSidebar.nuxt.test.ts b/web/app/components/NotificationSidebar.nuxt.test.ts index 424135d7..8253b2e9 100644 --- a/web/app/components/NotificationSidebar.nuxt.test.ts +++ b/web/app/components/NotificationSidebar.nuxt.test.ts @@ -62,7 +62,7 @@ describe("", { concurrent: false }, () => { const wrapper = await mountSidebar(true) - expect(wrapper.html()).toContain("grid-cols-[20rem]") + expect(wrapper.html()).toContain("grid-cols-[clamp(20rem,22dvw,26.25rem)]") }) it("slides the panel out of view while the inbox is closed", async ({ @@ -148,15 +148,21 @@ describe("", { concurrent: false }, () => { ).toEqual([[false]]) }) - it("closes the inbox when the box asks to be closed", async ({ expect }) => { - stubViewport(true) - const wrapper = await mountSidebar(true) + it.for([ + { name: "on narrow viewports", input: true }, + { name: "on wide viewports", input: false }, + ])( + "closes the inbox when the box asks to be closed $name", + async ({ input }, { expect }) => { + stubViewport(input) + const wrapper = await mountSidebar(true) - emitFrom(wrapper, "NotificationBox", "close-notification-box") - await nextTick() + emitFrom(wrapper, "NotificationBox", "close-notification-box") + await nextTick() - expect( - wrapper.findComponent(NotificationSidebar).emitted("update:modelValue"), - ).toEqual([[false]]) - }) + expect( + wrapper.findComponent(NotificationSidebar).emitted("update:modelValue"), + ).toEqual([[false]]) + }, + ) }) diff --git a/web/app/components/NotificationSidebar.vue b/web/app/components/NotificationSidebar.vue index b11586ca..5df9c305 100644 --- a/web/app/components/NotificationSidebar.vue +++ b/web/app/components/NotificationSidebar.vue @@ -31,12 +31,7 @@ const leftPosition = computed(() => { {{ $t("sidebar.notification-sheet.description") }} - +
{ :class=" cn( 'grid transition-[grid-template-columns] duration-200 ease-out', - open && sidebarOpen ? 'grid-cols-[20rem]' : 'grid-cols-[0rem]', + open && sidebarOpen + ? 'grid-cols-[clamp(20rem,22dvw,26.25rem)]' + : 'grid-cols-[0rem]', ) " > @@ -57,7 +54,10 @@ const leftPosition = computed(() => { " :style="{ left: `${leftPosition}px` }" > - +
diff --git a/web/app/components/editor/hooks/scheduled-reminder/ConfigMenu.vue b/web/app/components/editor/hooks/scheduled-reminder/ConfigMenu.vue index 0447ea80..203270b1 100644 --- a/web/app/components/editor/hooks/scheduled-reminder/ConfigMenu.vue +++ b/web/app/components/editor/hooks/scheduled-reminder/ConfigMenu.vue @@ -223,7 +223,7 @@ function close() { v-model:open="isSubOpen" :hook="props.hook" :diff="props.diff" - icon="lucide:timer" + icon="mingcute:stopwatch-line" :acknowledge-label="remindAgainLabel" :submit-label="isTriggered ? $t('editor.hooks.renew') : undefined" :submit-icon="isTriggered ? 'mingcute:check-fill' : undefined" diff --git a/web/app/components/notification.test.ts b/web/app/components/notification.test.ts new file mode 100644 index 00000000..7037ec94 --- /dev/null +++ b/web/app/components/notification.test.ts @@ -0,0 +1,384 @@ +import { describe, it } from "vitest" +import { + hookNotificationLine, + notificationDay, + notificationTimeLabel, +} from "./notification" +import { DocumentHookType } from "~/utils/api/document" +import { + NotificationCode, + type Notification, + type NotificationMetadataDocumentHookTriggered, +} from "~/utils/api/notification" + +// noon, so a notification from this morning and one from late last night +// fall on different calendar days +const NOW = new Date(2026, 2, 14, 12, 0, 0) +const GITHUB_SETTINGS = { + repository: "platform", + branch: "main", + paths: ["a.md"], +} + +describe("notificationDay", () => { + it.for([ + { + name: "puts a notification from this morning under today", + input: new Date(2026, 2, 14, 0, 5, 0), + expected: "today", + }, + { + name: "puts a notification dated ahead of the clock under today", + input: new Date(2026, 2, 15, 9, 0, 0), + expected: "today", + }, + { + name: "puts a notification from late last night under yesterday", + input: new Date(2026, 2, 13, 23, 55, 0), + expected: "yesterday", + }, + { + name: "puts a notification from two days ago under earlier", + input: new Date(2026, 2, 12, 23, 55, 0), + expected: "earlier", + }, + ])("$name", ({ input, expected }, { expect }) => { + expect(notificationDay(input, NOW)).toBe(expected) + }) + + it("reads a date the api sent as a string", ({ expect }) => { + expect( + notificationDay(new Date(2026, 2, 13, 8, 0, 0).toISOString(), NOW), + ).toBe("yesterday") + }) +}) + +describe("notificationTimeLabel", () => { + it.for([ + { + name: "says now within the first minute", + input: new Date(2026, 2, 14, 11, 59, 30), + expected: "notification.now-time-label", + }, + { + name: "counts minutes within the first hour", + input: new Date(2026, 2, 14, 11, 55, 0), + expected: 'notification.time.minutes:{"count":5}', + }, + { + name: "counts hours within the first day", + input: new Date(2026, 2, 14, 10, 0, 0), + expected: 'notification.time.hours:{"count":2}', + }, + { + name: "counts hours for last night", + input: new Date(2026, 2, 13, 23, 0, 0), + expected: 'notification.time.hours:{"count":13}', + }, + { + name: "counts one day once yesterday is a full day back", + input: new Date(2026, 2, 13, 9, 0, 0), + expected: 'notification.time.days:{"count":1}', + }, + { + name: "shows the date before yesterday", + input: new Date(2026, 2, 10, 9, 0, 0), + expected: "month-day-short:2026-03-10", + }, + ])("$name", ({ input, expected }, { expect }) => { + expect(notificationTimeLabel(input, NOW, fakeT, fakeD)).toBe(expected) + }) +}) + +describe("hookNotificationLine", () => { + it.for([ + { + name: "names the page a website hook watches", + input: { + type: DocumentHookType.URLWatcher, + hookSettings: { url: "https://nordpost.io/status/" }, + }, + expected: { + text: "notification.messages.hook-triggered.url-watcher", + detail: "nordpost.io/status", + }, + }, + { + name: "names the image a container hook watches", + input: { + type: DocumentHookType.ContainerImageWatcher, + hookSettings: { image: "ghcr.io/lastport/relay:2.14.0" }, + }, + expected: { + text: "notification.messages.hook-triggered.container-image-watcher", + detail: "ghcr.io/lastport/relay:2.14.0", + }, + }, + { + name: "counts the changed files of a GitHub hook", + input: { + type: DocumentHookType.GitHubTracking, + hookSettings: { + repository: "platform", + branch: "main", + paths: ["a.md", "b.md", "c.md"], + }, + githubTrackingChangedPaths: 2, + }, + expected: { + text: 'notification.messages.hook-triggered.github-tracking:{"changed":2,"total":3}', + detail: "main", + }, + }, + { + name: "leaves the count out for a GitHub hook without one", + input: { + type: DocumentHookType.GitHubTracking, + hookSettings: { + repository: "platform", + branch: "main", + paths: ["a.md"], + }, + githubTrackingChangedPaths: null, + }, + expected: { + text: "notification.messages.hook-triggered.github-tracking-fallback", + detail: "main", + }, + }, + { + name: "says when a reminder triggered", + input: { + type: DocumentHookType.ScheduledReminder, + hookSettings: { + scale: "linear", + duration: "168h", + schedule: new Date(2026, 2, 14, 9, 0, 0), + }, + }, + expected: { + text: 'editor.hooks.scheduled-reminder.subtext-triggered:{"date":"short-with-time:2026-03-14"}', + }, + }, + ])("$name", ({ input, expected }, { expect }) => { + expect( + hookNotificationLine( + makeNotification(NotificationCode.DocumentHookTriggered, input), + fakeT, + fakeD, + ), + ).toEqual(expected) + }) + + it.for([ + { + name: "names the page a website hook cannot reach", + input: { + type: DocumentHookType.URLWatcher, + status: "unreachable_url", + hookSettings: { url: "https://status.nordpost.io" }, + }, + expected: { + text: 'notification.messages.hook-attention.unreachable-url:{"url":"status.nordpost.io"}', + }, + }, + { + name: "says website change detection is not set up", + input: { + type: DocumentHookType.URLWatcher, + status: "unconfigured", + hookSettings: { url: "https://status.nordpost.io" }, + }, + expected: { + text: "notification.messages.hook-attention.url-watcher-unconfigured", + }, + }, + { + name: "says GitHub is not set up", + input: { + type: DocumentHookType.GitHubTracking, + status: "unconfigured", + hookSettings: GITHUB_SETTINGS, + }, + expected: { text: "editor.hooks.github-tracking.problems.unconfigured" }, + }, + { + name: "says GitHub is not connected", + input: { + type: DocumentHookType.GitHubTracking, + status: "missing_installation", + hookSettings: GITHUB_SETTINGS, + }, + expected: { + text: "editor.hooks.github-tracking.problems.missing-installation", + }, + }, + { + name: "names the repository that cannot be accessed", + input: { + type: DocumentHookType.GitHubTracking, + status: "missing_repository", + hookSettings: GITHUB_SETTINGS, + }, + expected: { + text: "notification.messages.hook-attention.missing-repository", + detail: "platform", + }, + }, + { + name: "names the branch that cannot be accessed", + input: { + type: DocumentHookType.GitHubTracking, + status: "missing_branch", + hookSettings: GITHUB_SETTINGS, + }, + expected: { + text: "notification.messages.hook-attention.missing-branch", + detail: "main", + }, + }, + { + name: "names the repository that is too large", + input: { + type: DocumentHookType.GitHubTracking, + status: "tree_truncated", + hookSettings: GITHUB_SETTINGS, + }, + expected: { + text: "notification.messages.hook-attention.tree-truncated", + detail: "platform", + }, + }, + { + name: "names the image a registry refused", + input: { + type: DocumentHookType.ContainerImageWatcher, + status: "unauthorized", + hookSettings: { image: "relay:2.14.0" }, + }, + expected: { + text: "notification.messages.hook-attention.unauthorized", + detail: "relay:2.14.0", + }, + }, + { + name: "names the image that no longer exists", + input: { + type: DocumentHookType.ContainerImageWatcher, + status: "image_not_found", + hookSettings: { image: "relay:2.14.0" }, + }, + expected: { + text: "notification.messages.hook-attention.image-not-found", + detail: "relay:2.14.0", + }, + }, + ])("$name", ({ input, expected }, { expect }) => { + expect( + hookNotificationLine( + makeNotification(NotificationCode.DocumentHookNeedsAttention, input), + fakeT, + fakeD, + ), + ).toEqual(expected) + }) + + it.for([ + { + name: "names the website hook of a trigger without settings", + input: { + code: NotificationCode.DocumentHookTriggered, + type: DocumentHookType.URLWatcher, + }, + expected: + 'notification.messages.document-hook-triggered-description:{"hook":"editor.hooks.url-watcher.title"}', + }, + { + name: "names the GitHub hook of a trigger without settings", + input: { + code: NotificationCode.DocumentHookTriggered, + type: DocumentHookType.GitHubTracking, + }, + expected: + 'notification.messages.document-hook-triggered-description:{"hook":"editor.hooks.github-tracking.title"}', + }, + { + name: "names the reminder hook of a trigger without settings", + input: { + code: NotificationCode.DocumentHookTriggered, + type: DocumentHookType.ScheduledReminder, + }, + expected: + 'notification.messages.document-hook-triggered-description:{"hook":"editor.hooks.scheduled-reminder.title"}', + }, + { + name: "names the container hook of a failed check without settings", + input: { + code: NotificationCode.DocumentHookNeedsAttention, + type: DocumentHookType.ContainerImageWatcher, + }, + expected: + 'notification.messages.document-hook-needs-attention-description:{"hook":"editor.hooks.container-image-watcher.title"}', + }, + { + name: "names a status this build does not know by its hook", + input: { + code: NotificationCode.DocumentHookNeedsAttention, + type: DocumentHookType.URLWatcher, + status: "future_status", + hookSettings: { url: "https://status.nordpost.io" }, + }, + expected: + 'notification.messages.document-hook-needs-attention-description:{"hook":"editor.hooks.url-watcher.title"}', + }, + { + name: "calls a hook type this build does not know unknown", + input: { + code: NotificationCode.DocumentHookTriggered, + type: "future-hook", + hookSettings: { url: "https://status.nordpost.io" }, + }, + expected: + 'notification.messages.document-hook-triggered-description:{"hook":"notification.hook-fallback"}', + }, + ])("$name", ({ input, expected }, { expect }) => { + const { code, ...metadata } = input + + expect( + hookNotificationLine(makeNotification(code, metadata), fakeT, fakeD), + ).toEqual({ text: expected }) + }) +}) + +// answers with the key and its params, so a test reads which message the +// helper picked and what it filled in +function fakeT(key: string, params?: Record) { + return params ? `${key}:${JSON.stringify(params)}` : key +} + +function fakeD(value: Date, format: string) { + const month = String(value.getMonth() + 1).padStart(2, "0") + const day = String(value.getDate()).padStart(2, "0") + + return `${format}:${String(value.getFullYear())}-${month}-${day}` +} + +function makeNotification( + code: NotificationCode, + metadata: Record, +): Notification { + return { + id: "notif-1", + userId: "user-1", + organizationId: "org-1", + code: code, + metadata: { + documentId: "doc-1", + branchId: "branch-1", + blockId: null, + ...metadata, + } as NotificationMetadataDocumentHookTriggered, + read: false, + createdAt: NOW, + } +} diff --git a/web/app/components/notification.ts b/web/app/components/notification.ts new file mode 100644 index 00000000..ba617186 --- /dev/null +++ b/web/app/components/notification.ts @@ -0,0 +1,247 @@ +import { + differenceInCalendarDays, + differenceInHours, + differenceInMinutes, +} from "date-fns" +import type { Notification } from "~/utils/api/notification" + +type Translate = (key: string, params?: Record) => string +type FormatDate = (value: Date, format: string) => string + +export const NOTIFICATION_DAYS = ["today", "yesterday", "earlier"] as const + +export type NotificationDay = (typeof NOTIFICATION_DAYS)[number] + +export interface NotificationActor { + id: string + name: string + image?: string | null +} + +export interface NotificationLine { + text: string + detail?: string +} + +// goes by the calendar day, not by the hours passed +export function notificationDay( + createdAt: Date | string, + now: Date, +): NotificationDay { + const days = differenceInCalendarDays(now, new Date(createdAt)) + if (days <= 0) { + return "today" + } + + return days === 1 ? "yesterday" : "earlier" +} + +export function notificationTimeLabel( + createdAt: Date | string, + now: Date, + t: Translate, + d: FormatDate, +): string { + const date = new Date(createdAt) + if (notificationDay(date, now) === "earlier") { + return d(date, "month-day-short") + } + + const minutes = differenceInMinutes(now, date) + if (minutes < 1) { + return t("notification.now-time-label") + } + + if (minutes < 60) { + return t("notification.time.minutes", { count: minutes }) + } + + const hours = differenceInHours(now, date) + if (hours < 24) { + return t("notification.time.hours", { count: hours }) + } + + return t("notification.time.days", { count: 1 }) +} + +export function hookNotificationLine( + notification: Notification, + t: Translate, + d: FormatDate, +): NotificationLine { + const metadata = + notification.metadata as NotificationMetadataDocumentHookTriggered + const needsAttention = + notification.code === NotificationCode.DocumentHookNeedsAttention + const line = needsAttention + ? attentionLine(metadata, t) + : triggeredLine(metadata, t, d) + if (line) { + return line + } + + const hook = hookTitle(metadata.type, t) + + return { + text: needsAttention + ? t("notification.messages.document-hook-needs-attention-description", { + hook: hook, + }) + : t("notification.messages.document-hook-triggered-description", { + hook: hook, + }), + } +} + +function triggeredLine( + metadata: NotificationMetadataDocumentHookTriggered, + t: Translate, + d: FormatDate, +): NotificationLine | null { + if (!metadata.hookSettings) { + return null + } + + switch (metadata.type) { + case DocumentHookType.URLWatcher: { + const settings = metadata.hookSettings as DocumentHookSettingsURLWatcher + + return { + text: t("notification.messages.hook-triggered.url-watcher"), + detail: displayURL(settings.url), + } + } + case DocumentHookType.ContainerImageWatcher: { + const settings = + metadata.hookSettings as DocumentHookSettingsContainerImageWatcher + + return { + text: t("notification.messages.hook-triggered.container-image-watcher"), + detail: settings.image, + } + } + case DocumentHookType.GitHubTracking: { + const settings = + metadata.hookSettings as DocumentHookSettingsGitHubTracking + + return { + text: metadata.githubTrackingChangedPaths + ? t("notification.messages.hook-triggered.github-tracking", { + changed: metadata.githubTrackingChangedPaths, + total: settings.paths.length, + }) + : t("notification.messages.hook-triggered.github-tracking-fallback"), + detail: settings.branch, + } + } + case DocumentHookType.ScheduledReminder: { + const settings = + metadata.hookSettings as DocumentHookSettingsScheduledReminder + + return { + text: t("editor.hooks.scheduled-reminder.subtext-triggered", { + date: d(new Date(settings.schedule), "short-with-time"), + }), + } + } + default: + return null + } +} + +function attentionLine( + metadata: NotificationMetadataDocumentHookTriggered, + t: Translate, +): NotificationLine | null { + if (!metadata.hookSettings) { + return null + } + + switch (metadata.status) { + case "unreachable_url": { + const settings = metadata.hookSettings as DocumentHookSettingsURLWatcher + + return { + text: t("notification.messages.hook-attention.unreachable-url", { + url: displayURL(settings.url), + }), + } + } + // both the website and the GitHub hook can be unconfigured + case "unconfigured": + return { + text: + metadata.type === DocumentHookType.GitHubTracking + ? t("editor.hooks.github-tracking.problems.unconfigured") + : t( + "notification.messages.hook-attention.url-watcher-unconfigured", + ), + } + case "missing_installation": + return { + text: t("editor.hooks.github-tracking.problems.missing-installation"), + } + case "missing_repository": { + const settings = + metadata.hookSettings as DocumentHookSettingsGitHubTracking + + return { + text: t("notification.messages.hook-attention.missing-repository"), + detail: settings.repository, + } + } + case "missing_branch": { + const settings = + metadata.hookSettings as DocumentHookSettingsGitHubTracking + + return { + text: t("notification.messages.hook-attention.missing-branch"), + detail: settings.branch, + } + } + case "tree_truncated": { + const settings = + metadata.hookSettings as DocumentHookSettingsGitHubTracking + + return { + text: t("notification.messages.hook-attention.tree-truncated"), + detail: settings.repository, + } + } + case "unauthorized": { + const settings = + metadata.hookSettings as DocumentHookSettingsContainerImageWatcher + + return { + text: t("notification.messages.hook-attention.unauthorized"), + detail: settings.image, + } + } + case "image_not_found": { + const settings = + metadata.hookSettings as DocumentHookSettingsContainerImageWatcher + + return { + text: t("notification.messages.hook-attention.image-not-found"), + detail: settings.image, + } + } + default: + return null + } +} + +function hookTitle(type: DocumentHookType, t: Translate): string { + switch (type) { + case DocumentHookType.URLWatcher: + return t("editor.hooks.url-watcher.title") + case DocumentHookType.GitHubTracking: + return t("editor.hooks.github-tracking.title") + case DocumentHookType.ScheduledReminder: + return t("editor.hooks.scheduled-reminder.title") + case DocumentHookType.ContainerImageWatcher: + return t("editor.hooks.container-image-watcher.title") + default: + return t("notification.hook-fallback") + } +} diff --git a/web/app/composables/api/useNotificationAPI.nuxt.test.ts b/web/app/composables/api/useNotificationAPI.nuxt.test.ts index 28d7a8a5..7a69e409 100644 --- a/web/app/composables/api/useNotificationAPI.nuxt.test.ts +++ b/web/app/composables/api/useNotificationAPI.nuxt.test.ts @@ -1,4 +1,4 @@ -import type { EntryKey } from "@pinia/colada" +import type { EntryKey, UseInfiniteQueryData } from "@pinia/colada" import { afterEach, beforeEach, describe, it } from "vitest" import { clearQueryCache, @@ -10,8 +10,10 @@ import { } from "./test-helpers" import useNotificationAPI from "./useNotificationAPI" -const LIST_KEY_A = ["notifications", "list", 10, 1] as const -const LIST_KEY_B = ["notifications", "list", 5, 2] as const +type NotificationPages = UseInfiniteQueryData + +const LIST_KEY_A = ["notifications", "list", 10] as const +const LIST_KEY_B = ["notifications", "list", 5] as const function makeNotificationAPI() { return runInApp(() => useNotificationAPI()) @@ -39,16 +41,28 @@ function makeNotification(id: string, read: boolean) { function makePage( notifications: NotificationsResponse["notifications"], + pageCount = 1, ): NotificationsResponse { - return { notifications, pageCount: 1 } + return { notifications, pageCount } } -function seedPage(key: EntryKey, page: NotificationsResponse) { - seedQueryData(key, page) +// what the cache holds for the given pages, loaded from the first one on +function makePages(...pages: NotificationsResponse[]): NotificationPages { + return { pages, pageParams: pages.map((_page, index) => index + 1) } } -function getPage(key: EntryKey) { - return readQueryData(key) as NotificationsResponse | undefined +function seedPages(key: EntryKey, ...pages: NotificationsResponse[]) { + seedQueryData(key, makePages(...pages)) +} + +function getPages(key: EntryKey) { + return readQueryData(key) as NotificationPages | undefined +} + +function readStates(key: EntryKey) { + return getPages(key)?.pages.map((page) => + page.notifications.map((n) => n.read), + ) } // creating a factory query eagerly loads it once; refresh() joins that @@ -62,19 +76,61 @@ describe("useNotificationAPI", { concurrent: false }, () => { afterEach(disposeMockEndpoints) describe("useFetchManyNotifications", () => { - it("fetches the requested notification page", async ({ expect }) => { + it("fetches the first notification page", async ({ expect }) => { const page = makePage([makeNotification("n1", false)]) const listCalls = mockEndpoint("GET", "/api/notifications", () => page) const api = makeNotificationAPI() - const list = runInApp(() => - api.useFetchManyNotifications({ limit: 10, page: 2 }), - ) + const list = runInApp(() => api.useFetchManyNotifications({ limit: 10 })) const result = await list.refresh() - expect(result.data).toEqual(page) + expect(result.data).toEqual(makePages(page)) + expect(list.hasNextPage.value).toBe(false) expect(listCalls).toHaveLength(1) - expect(listCalls[0]?.query).toEqual({ limit: "10", page: "2" }) + expect(listCalls[0]?.query).toEqual({ limit: "10", page: "1" }) + }) + + it("loads the next page while the server reports more", async ({ + expect, + }) => { + const listCalls = mockEndpoint("GET", "/api/notifications", (call) => + makePage([makeNotification(`n${String(call.query.page)}`, false)], 2), + ) + const api = makeNotificationAPI() + const list = runInApp(() => api.useFetchManyNotifications({ limit: 10 })) + await list.refresh() + expect(list.hasNextPage.value).toBe(true) + + await list.loadNextPage() + + expect(list.data.value).toEqual( + makePages( + makePage([makeNotification("n1", false)], 2), + makePage([makeNotification("n2", false)], 2), + ), + ) + expect(list.hasNextPage.value).toBe(false) + expect(listCalls.map((call) => call.query.page)).toEqual(["1", "2"]) + }) + + it("refetches every loaded page in order", async ({ expect }) => { + const listCalls = mockEndpoint("GET", "/api/notifications", (call) => + makePage([makeNotification(`n${String(call.query.page)}`, false)], 2), + ) + const api = makeNotificationAPI() + const list = runInApp(() => api.useFetchManyNotifications({ limit: 10 })) + await list.refresh() + await list.loadNextPage() + + await list.refetch() + + expect(list.data.value?.pages).toHaveLength(2) + expect(listCalls.map((call) => call.query.page)).toEqual([ + "1", + "2", + "1", + "2", + ]) }) }) @@ -121,22 +177,18 @@ describe("useNotificationAPI", { concurrent: false }, () => { "/api/notifications/read-status", () => ({}), ) - seedPage( + seedPages( LIST_KEY_A, makePage([makeNotification("n1", false), makeNotification("n2", true)]), + makePage([makeNotification("n4", false)]), ) - seedPage(LIST_KEY_B, makePage([makeNotification("n3", false)])) + seedPages(LIST_KEY_B, makePage([makeNotification("n3", false)])) const api = makeNotificationAPI() await api.markNotificationsRead.mutateAsync({ ids: [] }) - expect(getPage(LIST_KEY_A)?.notifications.map((n) => n.read)).toEqual([ - true, - true, - ]) - expect(getPage(LIST_KEY_B)?.notifications.map((n) => n.read)).toEqual([ - true, - ]) + expect(readStates(LIST_KEY_A)).toEqual([[true, true], [true]]) + expect(readStates(LIST_KEY_B)).toEqual([[true]]) expect(putCalls).toHaveLength(1) expect(putCalls[0]?.body).toEqual({ ids: [] }) // the success invalidation only refetches active queries — the @@ -163,27 +215,29 @@ describe("useNotificationAPI", { concurrent: false }, () => { "/api/notifications/read-status", () => ({}), ) - seedPage( + seedPages( LIST_KEY_A, makePage([ makeNotification("n1", false), makeNotification("n2", false), ]), + makePage([ + makeNotification("n4", false), + makeNotification("n5", false), + ]), ) - seedPage(LIST_KEY_B, makePage([makeNotification("n3", false)])) + seedPages(LIST_KEY_B, makePage([makeNotification("n3", false)])) const api = makeNotificationAPI() - await api.markNotificationsRead.mutateAsync({ ids: ["n1", "n3"] }) + await api.markNotificationsRead.mutateAsync({ ids: ["n1", "n3", "n5"] }) - expect(getPage(LIST_KEY_A)?.notifications.map((n) => n.read)).toEqual([ - true, - false, - ]) - expect(getPage(LIST_KEY_B)?.notifications.map((n) => n.read)).toEqual([ - true, + expect(readStates(LIST_KEY_A)).toEqual([ + [true, false], + [false, true], ]) + expect(readStates(LIST_KEY_B)).toEqual([[true]]) expect(putCalls).toHaveLength(1) - expect(putCalls[0]?.body).toEqual({ ids: ["n1", "n3"] }) + expect(putCalls[0]?.body).toEqual({ ids: ["n1", "n3", "n5"] }) expect(listCalls).toHaveLength(0) expect(countCalls).toHaveLength(0) }) @@ -208,19 +262,26 @@ describe("useNotificationAPI", { concurrent: false }, () => { throw createError({ statusCode: 500 }) }, ) - seedPage(LIST_KEY_A, makePage([makeNotification("n1", false)])) - seedPage(LIST_KEY_B, makePage([makeNotification("n2", false)])) + seedPages( + LIST_KEY_A, + makePage([makeNotification("n1", false)]), + makePage([makeNotification("n3", false)]), + ) + seedPages(LIST_KEY_B, makePage([makeNotification("n2", false)])) const api = makeNotificationAPI() await expect( api.markNotificationsRead.mutateAsync({ ids: [] }), ).rejects.toThrow() - expect(getPage(LIST_KEY_A)).toEqual( - makePage([makeNotification("n1", false)]), + expect(getPages(LIST_KEY_A)).toEqual( + makePages( + makePage([makeNotification("n1", false)]), + makePage([makeNotification("n3", false)]), + ), ) - expect(getPage(LIST_KEY_B)).toEqual( - makePage([makeNotification("n2", false)]), + expect(getPages(LIST_KEY_B)).toEqual( + makePages(makePage([makeNotification("n2", false)])), ) expect(putCalls).toHaveLength(1) expect(listCalls).toHaveLength(0) @@ -257,8 +318,8 @@ describe("useNotificationAPI", { concurrent: false }, () => { }) }, ) - seedPage(LIST_KEY_A, makePage([makeNotification("n1", false)])) - seedPage(LIST_KEY_B, makePage([makeNotification("n2", false)])) + seedPages(LIST_KEY_A, makePage([makeNotification("n1", false)])) + seedPages(LIST_KEY_B, makePage([makeNotification("n2", false)])) const api = makeNotificationAPI() const pending = api.markNotificationsRead.mutateAsync({ ids: [] }) @@ -266,20 +327,16 @@ describe("useNotificationAPI", { concurrent: false }, () => { // the optimistic update landed; divergent data written afterwards // must survive the failure - expect(getPage(LIST_KEY_A)?.notifications.map((n) => n.read)).toEqual([ - true, - ]) + expect(readStates(LIST_KEY_A)).toEqual([[true]]) const divergent = makePage([makeNotification("n9", false)]) - seedPage(LIST_KEY_A, divergent) + seedPages(LIST_KEY_A, divergent) rejectPut(createError({ statusCode: 500 })) await expect(pending).rejects.toThrow() - expect(getPage(LIST_KEY_A)).toEqual(divergent) - // the rollback is all-or-nothing, so the untouched page keeps its + expect(getPages(LIST_KEY_A)).toEqual(makePages(divergent)) + // the rollback is all-or-nothing, so the untouched entry keeps its // optimistic state too - expect(getPage(LIST_KEY_B)?.notifications.map((n) => n.read)).toEqual([ - true, - ]) + expect(readStates(LIST_KEY_B)).toEqual([[true]]) expect(putCalls).toHaveLength(1) expect(listCalls).toHaveLength(0) expect(countCalls).toHaveLength(0) @@ -318,28 +375,24 @@ describe("useNotificationAPI", { concurrent: false }, () => { }) }, ) - seedPage(LIST_KEY_B, makePage([makeNotification("n1", false)])) + seedPages(LIST_KEY_B, makePage([makeNotification("n1", false)])) const api = makeNotificationAPI() - const list = runInApp(() => - api.useFetchManyNotifications({ limit: 10, page: 1 }), - ) + const list = runInApp(() => api.useFetchManyNotifications({ limit: 10 })) const pending = api.markNotificationsRead.mutateAsync({ ids: [] }) await putReachedSignal // reaching the request proves the data-less entry did not abort - // the optimistic update; only the seeded page was touched - expect(getPage(LIST_KEY_A)).toBeUndefined() - expect(getPage(LIST_KEY_B)?.notifications.map((n) => n.read)).toEqual([ - true, - ]) + // the optimistic update; only the seeded entry was touched + expect(getPages(LIST_KEY_A)).toBeUndefined() + expect(readStates(LIST_KEY_B)).toEqual([[true]]) rejectPut(createError({ statusCode: 500 })) await expect(pending).rejects.toThrow() // the data-less entry is filtered from the rollback comparison - // too, so the rollback still restores the seeded page - expect(getPage(LIST_KEY_B)).toEqual( - makePage([makeNotification("n1", false)]), + // too, so the rollback still restores the seeded entry + expect(getPages(LIST_KEY_B)).toEqual( + makePages(makePage([makeNotification("n1", false)])), ) expect(putCalls).toHaveLength(1) expect(listCalls).toHaveLength(1) @@ -349,7 +402,7 @@ describe("useNotificationAPI", { concurrent: false }, () => { const resolved = makePage([makeNotification("n2", true)]) resolveList(resolved) const result = await list.refresh() - expect(result.data).toEqual(resolved) + expect(result.data).toEqual(makePages(resolved)) }) }) }) diff --git a/web/app/composables/api/useNotificationAPI.ts b/web/app/composables/api/useNotificationAPI.ts index c43cb7a2..cd2c4581 100644 --- a/web/app/composables/api/useNotificationAPI.ts +++ b/web/app/composables/api/useNotificationAPI.ts @@ -1,9 +1,10 @@ -import type { EntryKey } from "@pinia/colada" +import type { EntryKey, UseInfiniteQueryData } from "@pinia/colada" import isDeepEqual from "fast-deep-equal" +type NotificationPages = UseInfiniteQueryData + const NOTIFICATION_QUERY_KEYS = { - list: (limit: number, page: number) => - ["notifications", "list", limit, page] as const, + list: (limit: number) => ["notifications", "list", limit] as const, count: (read: boolean) => ["notifications", "count", read] as const, listRoot: ["notifications", "list"] as const, countRoot: ["notifications", "count"] as const, @@ -16,19 +17,12 @@ export default function () { function useFetchManyNotifications( paramsRef: MaybeRefOrGetter, ) { - return useQuery({ - key: () => { - const params = toValue(paramsRef) - const { limit, page } = params - - return NOTIFICATION_QUERY_KEYS.list(limit, page) - }, - query: async () => { - const params = toValue(paramsRef) - const { limit, page } = params + return useInfiniteQuery({ + key: () => NOTIFICATION_QUERY_KEYS.list(toValue(paramsRef).limit), + query: async ({ pageParam }) => { const searchParams = new URLSearchParams({ - limit: String(limit), - page: String(page), + limit: String(toValue(paramsRef).limit), + page: String(pageParam), }) return await $coreAPIClient( @@ -36,6 +30,9 @@ export default function () { { method: "GET" }, ) }, + initialPageParam: 1, + getNextPageParam: (lastPage, _pages, lastPageParam) => + lastPageParam < lastPage.pageCount ? lastPageParam + 1 : null, refetchOnMount: false, refetchOnWindowFocus: false, refetchOnReconnect: false, @@ -80,35 +77,37 @@ export default function () { const entries = queryCache.getEntries({ key: NOTIFICATION_QUERY_KEYS.listRoot, }) - // the key travels next to the page rather than inside it, so the + // the key travels next to the data rather than inside it, so the // bookkeeping never ends up in the cached data - const oldNotifs: { key: EntryKey; page: NotificationsResponse }[] = [] + const oldNotifs: { key: EntryKey; data: NotificationPages }[] = [] entries.forEach((entry) => { - const oldNotifPage = clone( - queryCache.getQueryData(entry.key), + const oldNotifData = clone( + queryCache.getQueryData(entry.key), ) // entries can exist without data (a query that has not // resolved yet); including them would make the update loop - // below throw on the missing notifications array and abort - // the whole mutation - if (!oldNotifPage) { + // below throw on the missing pages array and abort the whole + // mutation + if (!oldNotifData) { return } - oldNotifs.push({ key: clone(entry.key), page: oldNotifPage }) + oldNotifs.push({ key: clone(entry.key), data: oldNotifData }) }) const newNotifs = clone(oldNotifs) - newNotifs.forEach(({ key, page }) => { - page.notifications.forEach((notif) => { - if (req.ids.length === 0 || req.ids.includes(notif.id)) { - notif.read = true - } + newNotifs.forEach(({ key, data }) => { + data.pages.forEach((page) => { + page.notifications.forEach((notif) => { + if (req.ids.length === 0 || req.ids.includes(notif.id)) { + notif.read = true + } + }) }) - queryCache.setQueryData(key, page) + queryCache.setQueryData(key, data) queryCache.cancelQueries({ key }) }) @@ -132,20 +131,20 @@ export default function () { const entries = queryCache.getEntries({ key: NOTIFICATION_QUERY_KEYS.listRoot, }) - const cachedNotifs: { key: EntryKey; page: NotificationsResponse }[] = [] + const cachedNotifs: { key: EntryKey; data: NotificationPages }[] = [] entries.forEach((entry) => { - const cachedNotifPage = clone( - queryCache.getQueryData(entry.key), + const cachedNotifData = clone( + queryCache.getQueryData(entry.key), ) // mirror the onMutate filter so the rollback comparison sees - // the same set of pages the optimistic update touched - if (!cachedNotifPage) { + // the same set of entries the optimistic update touched + if (!cachedNotifData) { return } - cachedNotifs.push({ key: clone(entry.key), page: cachedNotifPage }) + cachedNotifs.push({ key: clone(entry.key), data: cachedNotifData }) }) if (!isDeepEqual(newNotifs, cachedNotifs)) { @@ -153,8 +152,8 @@ export default function () { } // rollback - oldNotifs?.forEach(({ key, page }) => { - queryCache.setQueryData(key, page) + oldNotifs?.forEach(({ key, data }) => { + queryCache.setQueryData(key, data) }) }, }) diff --git a/web/app/utils/api/notification.ts b/web/app/utils/api/notification.ts index bb338db6..5ed804fe 100644 --- a/web/app/utils/api/notification.ts +++ b/web/app/utils/api/notification.ts @@ -2,7 +2,6 @@ export const WS_NOTIFICATION_CREATION_TOPIC = "creation@notifications" export interface NotificationsParams { limit: number - page: number } export interface NotificationsCountParams { @@ -21,10 +20,11 @@ export interface Notification { export enum NotificationCode { DocumentReviewRequest = "notification.document.review_request", - DocumentHookTrigerred = "notification.document.hook_triggered", + DocumentHookTriggered = "notification.document.hook_triggered", DocumentHookNeedsAttention = "notification.document.hook_needs_attention", DocumentNewComment = "notification.document.new_comment", DocumentNewCommentReply = "notification.document.new_comment_reply", + DocumentCommentResolved = "notification.document.comment_resolved", } export type NotificationMetadata = @@ -34,6 +34,7 @@ export type NotificationMetadata = | NotificationMetadataDocumentNewCommentReply export interface NotificationMetadataDocumentReviewRequest { + userId: string documentId: string branchId: string } @@ -43,6 +44,9 @@ export interface NotificationMetadataDocumentHookTriggered { branchId: string blockId: string | null type: DocumentHookType + hookSettings?: DocumentHookSettings | null + status?: DocumentHookStatus + githubTrackingChangedPaths?: number | null } export interface NotificationMetadataDocumentNewComment { @@ -51,6 +55,7 @@ export interface NotificationMetadataDocumentNewComment { branchId: string commentId: string anchorBlockId: string | null + commentExcerpt?: string } export interface NotificationMetadataDocumentNewCommentReply { @@ -60,6 +65,7 @@ export interface NotificationMetadataDocumentNewCommentReply { commentId: string commentReplyId: string anchorBlockId: string | null + commentExcerpt?: string } export interface NotificationsResponse { diff --git a/web/i18n/locales/en/notification.json b/web/i18n/locales/en/notification.json index 6bb57f96..4a7347af 100644 --- a/web/i18n/locales/en/notification.json +++ b/web/i18n/locales/en/notification.json @@ -1,19 +1,46 @@ { "notification": { "inbox": "Inbox", - "read-all-button": "Read all", + "read-all-button": "Mark all as read", "empty-title": "All caught up", "empty-description": "You'll receive notifications here for page review invitations, triggered hooks, and more", "document-fallback": "Deleted Page", "hook-fallback": "Unknown", "now-time-label": "now", + "days": { + "today": "Today", + "yesterday": "Yesterday", + "earlier": "Earlier" + }, + "time": { + "minutes": "{count}m", + "hours": "{count}h", + "days": "{count}d" + }, "messages": { - "document-new-comment-description": "{user} has posted a new comment", - "document-new-comment-reply-description": "{user} has posted a reply to a comment", - "document-review-request-description": "Review requested for the draft version", + "separator": "·", + "document-new-comment-description": "{user} commented", + "document-new-comment-reply-description": "{user} replied", + "document-comment-resolved-description": "{user} resolved", + "document-review-request-description": "{user} requested your review", "document-hook-triggered-description": "{hook} hook was triggered", "document-hook-needs-attention-description": "{hook} hook needs attention", - "default-description": "You have a new notification" + "default-description": "You have a new notification", + "hook-triggered": { + "url-watcher": "Page changed", + "container-image-watcher": "Image updated", + "github-tracking": "{changed} of {total} watched files changed", + "github-tracking-fallback": "Watched files changed" + }, + "hook-attention": { + "unreachable-url": "Couldn't reach {url}", + "url-watcher-unconfigured": "Website change detection not set up", + "missing-repository": "Repository inaccessible", + "missing-branch": "Branch inaccessible", + "tree-truncated": "Repository too large", + "unauthorized": "Registry refused access", + "image-not-found": "Image not found" + } }, "actions": { "mark-read": "Mark as read", diff --git a/web/i18n/locales/en/shortcuts.json b/web/i18n/locales/en/shortcuts.json index e2bb9b85..e7ff1ef8 100644 --- a/web/i18n/locales/en/shortcuts.json +++ b/web/i18n/locales/en/shortcuts.json @@ -19,7 +19,7 @@ }, "toggle-inbox": { "label": "Toggle inbox", - "description": "Opens and closes the inbox on the right. Works anywhere in the workspace." + "description": "Opens and closes the inbox beside the sidebar. Works anywhere in the workspace." }, "search-for-documents": { "label": "Open search", diff --git a/web/i18n/locales/en/sidebar.json b/web/i18n/locales/en/sidebar.json index b40c7e7e..d9fb5fdc 100644 --- a/web/i18n/locales/en/sidebar.json +++ b/web/i18n/locales/en/sidebar.json @@ -81,8 +81,8 @@ "no-results": "No results found for {query}" }, "notification-sheet": { - "title": "Sidebar", - "description": "Displays the mobile sidebar." + "title": "Inbox", + "description": "Shows your notifications." } } }