amadan
feat: cell-anchored comments, which is what the commenter role is for
Spec 13 item 10, on the fallback it pre-decided: comments were to consume Docs' comment-thread primitive, Docs owns that design because its anchoring case is harder, and it has not shipped. Deciding the fallback in advance is what makes this a choice rather than a scramble — and adopting theirs later replaces the rendering, not the anchor. Anchored on stable row and column ids, never on A1. Spec 4.2 names this as one of the three things stable identity is load-bearing for: a comment stays attached to the cell it was written about when a row is inserted above it. An anchor of "B7" slides onto a different cell the first time anybody inserts a row. Body sealed, like every other piece of user content. A test greps the row. A thread is every comment sharing a cell, and resolving settles the whole thread: a thread is the unit people think in, and resolving one message of a conversation says nothing useful. Resolved threads are kept, because "we discussed this and settled it" is the most useful thing to read when the question comes back. This is what SheetCommenter is FOR. Until now the role granted nothing beyond reading, and a test pins both halves: a commenter can comment and still cannot write a cell. One bug caught while writing the tests: the resolve form posted stable ids where the handler reads positions. It would have resolved the wrong thread, or — far likelier, since an id and a position coincide only by accident — none at all, silently. The thread now carries both, and a test asserts the form posts the position.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018RN4HhHR4rHH3Fxh4LMds5
11 files changed,
+726
−1
internal/sheets/app.go+5 −0internal/sheets/comments.go+193 −0internal/sheets/handlers_comments.go+161 −0internal/sheets/handlers_comments_test.go+245 −0internal/sheets/migrations.go+1 −1internal/sheets/migrations/0006_create_comments.sql+4 −0internal/sheets/migrations/schema.sql+4 −0internal/sheets/models.go+34 −0internal/sheets/templates/comments.html+77 −0internal/sheets/templates/sheet.html+1 −0internal/sheets/templates/table.html+1 −0
CI passed — run details
diff --git a/internal/sheets/app.go b/internal/sheets/app.go| index 49c3815..3c6a113 100644 |
| --- a/internal/sheets/app.go |
| +++ b/internal/sheets/app.go |
| @@ -161,6 +161,11 @@ func (a *App) Router() http.Handler { |
| r.Get("/sheets/{sheetID}/history", a.handleHistory) |
| r.Post("/sheets/{sheetID}/restore", a.handleRestore) |
| r.Post("/sheets/{sheetID}/undo", a.handleUndo) |
| + // Comments. Reading needs a viewer's grant; writing needs |
| + // commenter, which is what that role is for. |
| + r.Get("/sheets/{sheetID}/comments", a.handleComments) |
| + r.Post("/sheets/{sheetID}/comments", a.handleCommentAdd) |
| + r.Post("/sheets/{sheetID}/comments/resolve", a.handleCommentResolve) |
| // The grid's own API. Under /api because it answers JSON and |
| // carries no chrome; behind the same session as everything |
diff --git a/internal/sheets/comments.go b/internal/sheets/comments.go| new file mode 100644 |
| index 0000000..0a8fa6e |
| --- /dev/null |
| +++ b/internal/sheets/comments.go |
| @@ -0,0 +1,193 @@ |
| +package sheets |
| + |
| +import ( |
| + "errors" |
| + "strings" |
| + "time" |
| +) |
| + |
| +// maxCommentRunes caps one comment. |
| +// |
| +// A note about a cell, not a document. Long enough for a real |
| +// paragraph, short enough that a thread stays readable beside the grid. |
| +const maxCommentRunes = 2000 |
| + |
| +// CommentEntry is one comment, unsealed and ready to render. |
| +type CommentEntry struct { |
| + ID uint64 |
| + Author string |
| + Body string |
| + When time.Time |
| + Resolved bool |
| +} |
| + |
| +// CommentThread is every comment on one cell. |
| +// |
| +// A thread is defined by the cell, not by a parent id. That is the |
| +// simplest structure that matches what people do — they discuss a cell |
| +// — and it is the one that survives adopting Docs' primitive later, |
| +// since their thread will still be anchored somewhere. |
| +type CommentThread struct { |
| + RowID, ColID uint64 |
| + // RowPos and ColPos are what a form posts back. The stable ids are |
| + // how the thread is anchored; the positions are how a person and |
| + // the routes address a cell, and the resolve form has to speak the |
| + // same language cellIDsFromForm reads. Sending the ids instead is |
| + // a mismatch that resolves the wrong thread — or, more likely, |
| + // none, since an id and a position collide only by accident. |
| + RowPos, ColPos int |
| + Ref string // A1 notation, for reading |
| + Entries []CommentEntry |
| + Resolved bool |
| +} |
| + |
| +// mayComment is the gate the comment routes use. |
| +// |
| +// Commenter is enough — that is the entire point of the role, which |
| +// until now granted nothing beyond reading. Editors and owners clear it |
| +// too, because Allows is a floor. |
| +func (a *App) mayComment(sheetID uint64, m Member) bool { |
| + role, ok := a.roleFor(sheetID, m) |
| + return ok && role.Allows(SheetCommenter) |
| +} |
| + |
| +// addComment writes a comment on a cell. |
| +func (a *App) addComment(tabID, rowID, colID uint64, body string, by Member) error { |
| + if a.db == nil { |
| + return errNoDB |
| + } |
| + if a.sealer == nil { |
| + return errNoSealer |
| + } |
| + clean := strings.TrimSpace(body) |
| + if clean == "" { |
| + return errors.New("sheets: a comment needs something in it") |
| + } |
| + if r := []rune(clean); len(r) > maxCommentRunes { |
| + clean = string(r[:maxCommentRunes]) |
| + } |
| + sealed, err := a.sealer.Seal("comment.body", []byte(clean)) |
| + if err != nil { |
| + return err |
| + } |
| + return a.db.Create(&Comment{ |
| + TabID: tabID, RowID: rowID, ColID: colID, |
| + AuthorID: by.ID, BodySealed: sealed, |
| + }).Error |
| +} |
| + |
| +// resolveThread marks every comment on a cell as settled. |
| +// |
| +// The whole cell, not one comment: a thread is the unit people think |
| +// in, and resolving one message of a conversation says nothing useful. |
| +func (a *App) resolveThread(tabID, rowID, colID uint64, by Member) error { |
| + if a.db == nil { |
| + return errNoDB |
| + } |
| + now := time.Now().UTC() |
| + return a.db.Model(&Comment{}). |
| + Where("tab_id = ? AND row_id = ? AND col_id = ? AND resolved_at IS NULL", |
| + tabID, rowID, colID). |
| + Updates(map[string]any{"resolved_at": now, "resolved_by": by.ID}).Error |
| +} |
| + |
| +// commentThreads reads every thread on a tab, unsealed. |
| +// |
| +// Threads are keyed by cell and returned in a stable order — resolved |
| +// last, then by first comment — so a page of them does not reshuffle |
| +// between loads. |
| +func (a *App) commentThreads(tabID uint64, idx posIndex) ([]CommentThread, error) { |
| + if a.db == nil { |
| + return nil, errNoDB |
| + } |
| + if a.sealer == nil { |
| + return nil, errNoSealer |
| + } |
| + var rows []Comment |
| + if err := a.db.Where("tab_id = ?", tabID).Order("id ASC").Find(&rows).Error; err != nil { |
| + return nil, err |
| + } |
| + names := a.memberNames() |
| + |
| + type key struct{ r, c uint64 } |
| + order := make([]key, 0, len(rows)) |
| + byCell := map[key]*CommentThread{} |
| + |
| + for _, c := range rows { |
| + body, err := a.sealer.Open("comment.body", c.BodySealed) |
| + if err != nil { |
| + // One unreadable comment must not take the thread with it. |
| + continue |
| + } |
| + k := key{c.RowID, c.ColID} |
| + th, ok := byCell[k] |
| + if !ok { |
| + rp, okR := idx.rowPos[c.RowID] |
| + cp, okC := idx.colPos[c.ColID] |
| + if !okR || !okC { |
| + // A comment whose row or column is gone has nothing to |
| + // point at. Skipped rather than shown against a |
| + // coordinate we would have to invent. |
| + continue |
| + } |
| + th = &CommentThread{ |
| + RowID: c.RowID, ColID: c.ColID, |
| + RowPos: rp, ColPos: cp, |
| + Ref: colLabel(cp) + itoaPos(rp+1), Resolved: true, |
| + } |
| + byCell[k] = th |
| + order = append(order, k) |
| + } |
| + th.Entries = append(th.Entries, CommentEntry{ |
| + ID: c.ID, Author: names[c.AuthorID], Body: string(body), |
| + When: c.CreatedAt, Resolved: c.ResolvedAt != nil, |
| + }) |
| + if c.ResolvedAt == nil { |
| + th.Resolved = false |
| + } |
| + } |
| + |
| + out := make([]CommentThread, 0, len(order)) |
| + for _, k := range order { |
| + if !byCell[k].Resolved { |
| + out = append(out, *byCell[k]) |
| + } |
| + } |
| + for _, k := range order { |
| + if byCell[k].Resolved { |
| + out = append(out, *byCell[k]) |
| + } |
| + } |
| + return out, nil |
| +} |
| + |
| +// commentedCells reports which cells carry an unresolved thread, so the |
| +// grid and table can mark them. |
| +func (a *App) commentedCells(tabID uint64) map[[2]uint64]bool { |
| + out := map[[2]uint64]bool{} |
| + if a.db == nil { |
| + return out |
| + } |
| + var rows []Comment |
| + if err := a.db.Where("tab_id = ? AND resolved_at IS NULL", tabID).Find(&rows).Error; err != nil { |
| + return out |
| + } |
| + for _, c := range rows { |
| + out[[2]uint64{c.RowID, c.ColID}] = true |
| + } |
| + return out |
| +} |
| + |
| +// itoaPos is strconv.Itoa, named here so the coordinate-building code |
| +// reads the same as colLabel beside it. |
| +func itoaPos(n int) string { |
| + if n == 0 { |
| + return "0" |
| + } |
| + var b []byte |
| + for n > 0 { |
| + b = append([]byte{byte('0' + n%10)}, b...) |
| + n /= 10 |
| + } |
| + return string(b) |
| +} |
diff --git a/internal/sheets/handlers_comments.go b/internal/sheets/handlers_comments.go| new file mode 100644 |
| index 0000000..aa52f1c |
| --- /dev/null |
| +++ b/internal/sheets/handlers_comments.go |
| @@ -0,0 +1,161 @@ |
| +package sheets |
| + |
| +import ( |
| + "net/http" |
| + "strconv" |
| + |
| + "amadan.net/oficina/sheets/internal/sheets/store" |
| +) |
| + |
| +type commentsView struct { |
| + Sheet SheetSummary |
| + Threads []CommentThread |
| + MayComment bool |
| + Notice string |
| +} |
| + |
| +func commentNotice(code string) string { |
| + switch code { |
| + case "added": |
| + return "Comment added." |
| + case "resolved": |
| + return "Thread resolved. It stays here, so the conversation is not lost." |
| + case "empty": |
| + return "A comment needs something in it." |
| + case "no-cell": |
| + return "That cell is not part of this sheet." |
| + default: |
| + return "" |
| + } |
| +} |
| + |
| +// handleComments lists every thread on a sheet. |
| +// |
| +// Reading needs only a viewer's grant: a comment is about the sheet, |
| +// and somebody who can read the cells can read what was said about |
| +// them. Writing needs commenter, which is what that role is for. |
| +func (a *App) handleComments(w http.ResponseWriter, r *http.Request) { |
| + s, ok := a.sheetFromURL(w, r) |
| + if !ok { |
| + return |
| + } |
| + viewer, ok := a.actor(r) |
| + if !ok { |
| + a.notFound(w, r) |
| + return |
| + } |
| + h, err := a.openTabFor(s) |
| + if err != nil { |
| + a.fail(w, r, err) |
| + return |
| + } |
| + defer h.Release() |
| + |
| + threads, err := a.commentThreads(s.TabID, newPosIndex(h)) |
| + if err != nil { |
| + a.fail(w, r, err) |
| + return |
| + } |
| + a.render(w, r, "comments", "Comments — "+s.Title, http.StatusOK, commentsView{ |
| + Sheet: s, Threads: threads, |
| + MayComment: a.mayComment(s.ID, viewer), |
| + Notice: commentNotice(r.URL.Query().Get("notice")), |
| + }) |
| +} |
| + |
| +// handleCommentAdd writes a comment about a cell. |
| +func (a *App) handleCommentAdd(w http.ResponseWriter, r *http.Request) { |
| + s, viewer, h, ok := a.commentTarget(w, r) |
| + if !ok { |
| + return |
| + } |
| + defer h.Release() |
| + |
| + rowID, colID, ok := a.cellIDsFromForm(r, h) |
| + if !ok { |
| + a.commentRedirect(w, r, s, "no-cell") |
| + return |
| + } |
| + if err := a.addComment(s.TabID, rowID, colID, r.FormValue("body"), viewer); err != nil { |
| + // The body is not echoed. It is user content, and an error |
| + // string is a log line waiting to happen (AGENTS.md). |
| + a.commentRedirect(w, r, s, "empty") |
| + return |
| + } |
| + a.commentRedirect(w, r, s, "added") |
| +} |
| + |
| +// handleCommentResolve settles a thread. |
| +func (a *App) handleCommentResolve(w http.ResponseWriter, r *http.Request) { |
| + s, viewer, h, ok := a.commentTarget(w, r) |
| + if !ok { |
| + return |
| + } |
| + defer h.Release() |
| + |
| + rowID, colID, ok := a.cellIDsFromForm(r, h) |
| + if !ok { |
| + a.commentRedirect(w, r, s, "no-cell") |
| + return |
| + } |
| + if err := a.resolveThread(s.TabID, rowID, colID, viewer); err != nil { |
| + a.fail(w, r, err) |
| + return |
| + } |
| + a.commentRedirect(w, r, s, "resolved") |
| +} |
| + |
| +// commentTarget resolves the sheet, proves the viewer may comment, and |
| +// opens the tab. The caller must Release the handle. |
| +func (a *App) commentTarget(w http.ResponseWriter, r *http.Request) (SheetSummary, Member, *store.Handle, bool) { |
| + s, ok := a.sheetFromURL(w, r) |
| + if !ok { |
| + return SheetSummary{}, Member{}, nil, false |
| + } |
| + viewer, ok := a.actor(r) |
| + if !ok { |
| + a.notFound(w, r) |
| + return SheetSummary{}, Member{}, nil, false |
| + } |
| + if !a.mayComment(s.ID, viewer) { |
| + // 404 like every other refusal: a viewer who cannot comment |
| + // learns nothing about who can. |
| + a.notFound(w, r) |
| + return SheetSummary{}, Member{}, nil, false |
| + } |
| + h, err := a.openTabFor(s) |
| + if err != nil { |
| + a.fail(w, r, err) |
| + return SheetSummary{}, Member{}, nil, false |
| + } |
| + return s, viewer, h, true |
| +} |
| + |
| +// cellIDsFromForm turns the r/c a form submitted into stable ids. |
| +// |
| +// EnsureRow/EnsureCol, so a comment can be left on a cell nobody has |
| +// typed into yet — which is a real thing to want ("this column should |
| +// hold X"). It is also why comments anchor on ids: the row exists from |
| +// the moment somebody talks about it. |
| +func (a *App) cellIDsFromForm(r *http.Request, h *store.Handle) (uint64, uint64, bool) { |
| + row, err1 := strconv.Atoi(r.FormValue("r")) |
| + col, err2 := strconv.Atoi(r.FormValue("c")) |
| + if err1 != nil || err2 != nil || row < 0 || col < 0 || |
| + row >= maxWindowRows || col >= tableCols { |
| + return 0, 0, false |
| + } |
| + rowID, err := h.EnsureRow(row) |
| + if err != nil { |
| + return 0, 0, false |
| + } |
| + colID, err := h.EnsureCol(col) |
| + if err != nil { |
| + return 0, 0, false |
| + } |
| + return rowID, colID, true |
| +} |
| + |
| +func (a *App) commentRedirect(w http.ResponseWriter, r *http.Request, s SheetSummary, notice string) { |
| + http.Redirect(w, r, "/sheets/"+strconv.FormatUint(s.ID, 10)+"/comments?notice="+notice, |
| + http.StatusSeeOther) |
| +} |
diff --git a/internal/sheets/handlers_comments_test.go b/internal/sheets/handlers_comments_test.go| new file mode 100644 |
| index 0000000..c94f15f |
| --- /dev/null |
| +++ b/internal/sheets/handlers_comments_test.go |
| @@ -0,0 +1,245 @@ |
| +package sheets |
| + |
| +import ( |
| + "bytes" |
| + "fmt" |
| + "net/http" |
| + "net/http/httptest" |
| + "net/url" |
| + "strings" |
| + "testing" |
| +) |
| + |
| +func commentsPath(s SheetSummary) string { return fmt.Sprintf("/sheets/%d/comments", s.ID) } |
| + |
| +func addComment(t *testing.T, app *App, s SheetSummary, cookies []*http.Cookie, r, c int, body string) *httptest.ResponseRecorder { |
| + t.Helper() |
| + form := url.Values{"r": {fmt.Sprint(r)}, "c": {fmt.Sprint(c)}, "body": {body}} |
| + return do(t, app, as(postForm(commentsPath(s), form), cookies)) |
| +} |
| + |
| +func commentsBody(t *testing.T, app *App, s SheetSummary, cookies []*http.Cookie) string { |
| + t.Helper() |
| + return do(t, app, as(httptest.NewRequest(http.MethodGet, commentsPath(s), nil), cookies)).Body.String() |
| +} |
| + |
| +func TestACommentIsWrittenAndShown(t *testing.T) { |
| + app, cookies := signIn(t) |
| + s, _ := newSheet(t, app, cookies, "Budget") |
| + |
| + if rec := addComment(t, app, s, cookies, 0, 0, "Is this figure current?"); rec.Code != http.StatusSeeOther { |
| + t.Fatalf("adding a comment = %d: %s", rec.Code, rec.Body.String()) |
| + } |
| + body := commentsBody(t, app, s, cookies) |
| + if !strings.Contains(body, "Is this figure current?") { |
| + t.Error("the comment is not shown") |
| + } |
| + if !strings.Contains(body, "A1") { |
| + t.Error("the comment is not anchored to a readable cell reference") |
| + } |
| +} |
| + |
| +// The one rule. Comment text is user content — spec §2.1 lists it |
| +// beside cell values and sheet titles. |
| +func TestCommentTextIsSealedAtRest(t *testing.T) { |
| + app, cookies := signIn(t) |
| + s, _ := newSheet(t, app, cookies, "Budget") |
| + |
| + const secret = "SalaryLooksWrongToMe" |
| + addComment(t, app, s, cookies, 0, 0, secret) |
| + |
| + var raw []byte |
| + if err := app.db.Raw("SELECT body_sealed FROM comments LIMIT 1").Row().Scan(&raw); err != nil { |
| + t.Fatalf("read back: %v", err) |
| + } |
| + if len(raw) == 0 { |
| + t.Fatal("no sealed body was stored") |
| + } |
| + if bytes.Contains(raw, []byte(secret)) { |
| + t.Fatal("the comment reached the database in plaintext") |
| + } |
| +} |
| + |
| +// This is what the commenter role is FOR. Until comments existed it |
| +// granted nothing beyond reading. |
| +func TestACommenterCanCommentButNotEdit(t *testing.T) { |
| + app, cookies := signIn(t) |
| + s, cells := newSheet(t, app, cookies, "Budget") |
| + guestOn(t, app, s.ID, "ada@elsewhere.example", SheetCommenter) |
| + adaCookies := signInAs(t, app, "ada@elsewhere.example") |
| + |
| + if rec := addComment(t, app, s, adaCookies, 0, 0, "a note from Ada"); rec.Code != http.StatusSeeOther { |
| + t.Fatalf("a commenter could not comment: %d", rec.Code) |
| + } |
| + if !strings.Contains(commentsBody(t, app, s, cookies), "a note from Ada") { |
| + t.Error("the commenter's note is not visible to the owner") |
| + } |
| + // And still cannot write a cell. |
| + if rec := postCell(t, app, adaCookies, cells, `{"r":0,"c":0,"raw":"sneaky"}`); rec.Code == http.StatusOK { |
| + t.Error("a commenter wrote a cell") |
| + } |
| +} |
| + |
| +func TestAViewerCannotComment(t *testing.T) { |
| + app, cookies := signIn(t) |
| + s, _ := newSheet(t, app, cookies, "Budget") |
| + guestOn(t, app, s.ID, "ada@elsewhere.example", SheetViewer) |
| + adaCookies := signInAs(t, app, "ada@elsewhere.example") |
| + |
| + if rec := addComment(t, app, s, adaCookies, 0, 0, "sneaky"); rec.Code != http.StatusNotFound { |
| + t.Fatalf("a viewer commented: %d", rec.Code) |
| + } |
| + // A viewer may still READ comments: a comment is about the sheet, |
| + // and somebody who can read the cells can read what was said. |
| + addComment(t, app, s, cookies, 0, 0, "owner's note") |
| + if !strings.Contains(commentsBody(t, app, s, adaCookies), "owner's note") { |
| + t.Error("a viewer cannot read comments on a sheet they can read") |
| + } |
| +} |
| + |
| +func TestCommentsAreGated(t *testing.T) { |
| + app, cookies := signIn(t) |
| + private, _ := newSheet(t, app, cookies, "Private") |
| + addComment(t, app, private, cookies, 0, 0, "confidential note") |
| + other, _ := newSheet(t, app, cookies, "Shared") |
| + guestOn(t, app, other.ID, "ada@elsewhere.example", SheetCommenter) |
| + adaCookies := signInAs(t, app, "ada@elsewhere.example") |
| + |
| + rec := do(t, app, as(httptest.NewRequest(http.MethodGet, commentsPath(private), nil), adaCookies)) |
| + if rec.Code != http.StatusNotFound { |
| + t.Fatalf("a guest read another sheet's comments: %d", rec.Code) |
| + } |
| + if strings.Contains(rec.Body.String(), "confidential") { |
| + t.Fatal("the refusal leaked comment text") |
| + } |
| +} |
| + |
| +// Every comment on a cell is one thread, and resolving settles the |
| +// whole thread rather than one message. |
| +func TestCommentsOnOneCellFormOneThread(t *testing.T) { |
| + app, cookies := signIn(t) |
| + s, _ := newSheet(t, app, cookies, "Budget") |
| + |
| + addComment(t, app, s, cookies, 1, 1, "first") |
| + addComment(t, app, s, cookies, 1, 1, "second") |
| + addComment(t, app, s, cookies, 2, 0, "elsewhere") |
| + |
| + h, err := app.openTabFor(s) |
| + if err != nil { |
| + t.Fatalf("open tab: %v", err) |
| + } |
| + defer h.Release() |
| + threads, err := app.commentThreads(s.TabID, newPosIndex(h)) |
| + if err != nil { |
| + t.Fatalf("commentThreads: %v", err) |
| + } |
| + if len(threads) != 2 { |
| + t.Fatalf("got %d threads, want 2 — comments on one cell are one thread", len(threads)) |
| + } |
| + for _, th := range threads { |
| + if th.Ref == "B2" && len(th.Entries) != 2 { |
| + t.Fatalf("B2's thread holds %d comments, want 2", len(th.Entries)) |
| + } |
| + } |
| +} |
| + |
| +func TestResolvingSettlesTheWholeThread(t *testing.T) { |
| + app, cookies := signIn(t) |
| + s, _ := newSheet(t, app, cookies, "Budget") |
| + addComment(t, app, s, cookies, 0, 0, "one") |
| + addComment(t, app, s, cookies, 0, 0, "two") |
| + |
| + form := url.Values{"r": {"0"}, "c": {"0"}} |
| + rec := do(t, app, as(postForm(commentsPath(s)+"/resolve", form), cookies)) |
| + if rec.Code != http.StatusSeeOther { |
| + t.Fatalf("resolve = %d", rec.Code) |
| + } |
| + |
| + body := commentsBody(t, app, s, cookies) |
| + if !strings.Contains(body, "resolved") { |
| + t.Error("the thread is not marked resolved") |
| + } |
| + // Kept, not deleted: the conversation is worth reading later. |
| + for _, want := range []string{"one", "two"} { |
| + if !strings.Contains(body, want) { |
| + t.Errorf("resolving lost the comment %q", want) |
| + } |
| + } |
| +} |
| + |
| +// Anchored on stable ids, so a comment stays on the cell it was written |
| +// about. This is one of the three things spec §4.2 says stable identity |
| +// is load-bearing for. |
| +func TestACommentAnchorsToTheCellNotTheCoordinate(t *testing.T) { |
| + app, cookies := signIn(t) |
| + s, _ := newSheet(t, app, cookies, "Budget") |
| + addComment(t, app, s, cookies, 3, 0, "about this row") |
| + |
| + var c Comment |
| + if err := app.db.First(&c).Error; err != nil { |
| + t.Fatalf("read comment: %v", err) |
| + } |
| + if c.RowID == 0 || c.ColID == 0 { |
| + t.Fatal("the comment carries no stable ids") |
| + } |
| + // The stored anchor must not be the display position: those are 3 |
| + // and 0, and storing them would slide the comment onto another cell |
| + // the first time a row was inserted above it. |
| + h, err := app.openTabFor(s) |
| + if err != nil { |
| + t.Fatalf("open tab: %v", err) |
| + } |
| + defer h.Release() |
| + idx := newPosIndex(h) |
| + if idx.rowPos[c.RowID] != 3 { |
| + t.Fatalf("the comment's row id resolves to position %d, want 3", idx.rowPos[c.RowID]) |
| + } |
| +} |
| + |
| +func TestAnEmptyCommentIsRefused(t *testing.T) { |
| + app, cookies := signIn(t) |
| + s, _ := newSheet(t, app, cookies, "Budget") |
| + |
| + rec := addComment(t, app, s, cookies, 0, 0, " ") |
| + if loc := rec.Header().Get("Location"); !strings.Contains(loc, "empty") { |
| + t.Fatalf("Location = %q, want the empty refusal", loc) |
| + } |
| + var n int64 |
| + app.db.Model(&Comment{}).Count(&n) |
| + if n != 0 { |
| + t.Fatalf("an empty comment was stored (%d rows)", n) |
| + } |
| +} |
| + |
| +func TestCommentsAreScriptless(t *testing.T) { |
| + app, cookies := signIn(t) |
| + s, _ := newSheet(t, app, cookies, "Budget") |
| + addComment(t, app, s, cookies, 0, 0, "a note") |
| + |
| + body := withoutScript(commentsBody(t, app, s, cookies)) |
| + for _, want := range []string{ |
| + fmt.Sprintf(`action="/sheets/%d/comments"`, s.ID), |
| + `name="body"`, |
| + fmt.Sprintf(`action="/sheets/%d/comments/resolve"`, s.ID), |
| + } { |
| + if !strings.Contains(body, want) { |
| + t.Errorf("with scripts stripped, %q is gone", want) |
| + } |
| + } |
| +} |
| + |
| +// The resolve form has to speak the same language cellIDsFromForm |
| +// reads. Posting stable ids where positions are expected resolves the |
| +// wrong thread, or none. |
| +func TestTheResolveFormPostsPositionsNotIDs(t *testing.T) { |
| + app, cookies := signIn(t) |
| + s, _ := newSheet(t, app, cookies, "Budget") |
| + // Row 5 so the position and the row id cannot coincide. |
| + addComment(t, app, s, cookies, 5, 0, "a note") |
| + |
| + body := commentsBody(t, app, s, cookies) |
| + if !strings.Contains(body, `name="r" value="5"`) { |
| + t.Fatalf("the resolve form does not post the row POSITION:\n%s", |
| + body[max(0, strings.Index(body, "resolve")-300):]) |
| + } |
| +} |
diff --git a/internal/sheets/migrations.go b/internal/sheets/migrations.go| index 1525419..1036731 100644 |
| --- a/internal/sheets/migrations.go |
| +++ b/internal/sheets/migrations.go |
| @@ -50,5 +50,5 @@ var BootSchema = migrate.Merge(sessions.Schema, auth.Schema, Schema) |
| // moved. |
| var Models = []any{ |
| store.Sheet{}, store.Tab{}, store.Row{}, store.Col{}, |
| - store.Cell{}, store.Format{}, store.Event{}, Member{}, Grant{}, ShareLink{}, |
| + store.Cell{}, store.Format{}, store.Event{}, Member{}, Grant{}, ShareLink{}, Comment{}, |
| } |
diff --git a/internal/sheets/migrations/0006_create_comments.sql b/internal/sheets/migrations/0006_create_comments.sql| new file mode 100644 |
| index 0000000..54a7e2e |
| --- /dev/null |
| +++ b/internal/sheets/migrations/0006_create_comments.sql |
| @@ -0,0 +1,4 @@ |
| +CREATE TABLE `comments` (`id` integer PRIMARY KEY AUTOINCREMENT,`tab_id` integer,`row_id` integer,`col_id` integer,`author_id` integer,`body_sealed` blob,`created_at` datetime,`resolved_at` datetime,`resolved_by` integer); |
| + |
| +CREATE INDEX `idx_comments_tab_id` ON `comments`(`tab_id`); |
| + |
diff --git a/internal/sheets/migrations/schema.sql b/internal/sheets/migrations/schema.sql| index b8b0bcc..509c9ac 100644 |
| --- a/internal/sheets/migrations/schema.sql |
| +++ b/internal/sheets/migrations/schema.sql |
| @@ -5,6 +5,8 @@ CREATE TABLE `cells` (`tab_id` integer,`row_id` integer,`col_id` integer,`input_ |
| CREATE TABLE `cols` (`id` integer PRIMARY KEY AUTOINCREMENT,`tab_id` integer,`position` integer,`width` integer); |
| +CREATE TABLE `comments` (`id` integer PRIMARY KEY AUTOINCREMENT,`tab_id` integer,`row_id` integer,`col_id` integer,`author_id` integer,`body_sealed` blob,`created_at` datetime,`resolved_at` datetime,`resolved_by` integer); |
| + |
| CREATE TABLE `events` (`id` integer PRIMARY KEY AUTOINCREMENT,`kind` text,`tab_id` integer,`actor_id` integer,`payload_sealed` blob,`trigger_id` integer,`derived_count` integer,`created_at` datetime); |
| CREATE TABLE `formats` (`id` integer PRIMARY KEY AUTOINCREMENT,`digest` text,`number_fmt` text,`bold` numeric,`italic` numeric,`fill_intent_h` real,`fill_intent_c` real,`fill_hex` text,`align` text, `ink_hex` text); |
| @@ -23,6 +25,8 @@ CREATE TABLE `tabs` (`id` integer PRIMARY KEY AUTOINCREMENT,`sheet_id` integer,` |
| CREATE UNIQUE INDEX `idx_col_tab_pos` ON `cols`(`tab_id`,`position`); |
| +CREATE INDEX `idx_comments_tab_id` ON `comments`(`tab_id`); |
| + |
| CREATE INDEX `idx_events_kind` ON `events`(`kind`); |
| CREATE INDEX `idx_events_tab_id` ON `events`(`tab_id`); |
diff --git a/internal/sheets/models.go b/internal/sheets/models.go| index ebe2002..ff754e2 100644 |
| --- a/internal/sheets/models.go |
| +++ b/internal/sheets/models.go |
| @@ -144,3 +144,37 @@ type ShareLink struct { |
| CreatedAt time.Time |
| RevokedAt *time.Time |
| } |
| + |
| +// Comment is a note somebody wrote about one cell. |
| +// |
| +// **Anchored on stable row and column ids, never on A1 coordinates.** |
| +// Spec §4.2 names this as one of the three things stable identity is |
| +// load-bearing for: a comment stays attached to the cell it was written |
| +// about when a row is inserted above it. An anchor of "B7" would slide |
| +// onto a different cell the first time anybody inserted a row. |
| +// |
| +// BodySealed because comment text is user content — spec §2.1 lists it |
| +// beside cell values and sheet titles. |
| +// |
| +// A thread is every comment sharing a cell, ordered by id. That is the |
| +// **local** implementation §13 pre-decided as the fallback: comments |
| +// were to consume Docs' comment-thread primitive, Docs owns that design |
| +// because its anchoring case is harder, and it has not shipped. Deciding |
| +// the fallback in advance is what makes this a choice rather than a |
| +// scramble; adopting theirs later replaces the rendering, not the |
| +// anchor. |
| +// |
| +// ResolvedAt marks a thread dealt with. Nulled rather than deleted: |
| +// "we discussed this and settled it" is worth keeping, and a resolved |
| +// thread is the most useful thing to read when the same question comes |
| +// back. |
| +type Comment struct { |
| + ID uint64 `gorm:"primaryKey"` |
| + TabID uint64 `gorm:"index"` |
| + RowID, ColID uint64 |
| + AuthorID uint64 |
| + BodySealed []byte // seal purpose "comment.body" |
| + CreatedAt time.Time |
| + ResolvedAt *time.Time |
| + ResolvedBy uint64 |
| +} |
diff --git a/internal/sheets/templates/comments.html b/internal/sheets/templates/comments.html| new file mode 100644 |
| index 0000000..90b1e1a |
| --- /dev/null |
| +++ b/internal/sheets/templates/comments.html |
| @@ -0,0 +1,77 @@ |
| +{{define "content"}} |
| +<header rst-page-header> |
| +<div rst-page-header-titles> |
| +<h1>Comments — {{.Data.Sheet.Title}}</h1> |
| +<p rst-page-header-sub>Notes about particular cells. Open threads first.</p> |
| +</div> |
| +<a rst-btn="ghost" href="/sheets/{{.Data.Sheet.ID}}">Back to the sheet</a> |
| +</header> |
| + |
| +{{with .Data.Notice}} |
| +<div rst-callout><div rst-callout-body><p>{{.}}</p></div></div> |
| +{{end}} |
| + |
| +{{if .Data.MayComment}} |
| +<form rst-form method="post" action="/sheets/{{.Data.Sheet.ID}}/comments"> |
| +<div rst-field-row> |
| +<div rst-field> |
| +<label rst-field-label for="r">Row</label> |
| +<input rst-input="short" id="r" name="r" type="number" min="0" value="0" required> |
| +</div> |
| +<div rst-field> |
| +<label rst-field-label for="c">Column</label> |
| +<input rst-input="short" id="c" name="c" type="number" min="0" value="0" required> |
| +</div> |
| +</div> |
| +<div rst-field> |
| +<label rst-field-label for="body">Comment</label> |
| +<textarea rst-textarea id="body" name="body" rows="3" required></textarea> |
| +</div> |
| +<div rst-form-actions> |
| +<button rst-btn="primary" type="submit">Add comment</button> |
| +</div> |
| +</form> |
| +<p rst-help>Row and column are counted from zero, the way the grid |
| +addresses them: row 0, column 0 is A1.</p> |
| +{{end}} |
| + |
| +{{if .Data.Threads}} |
| +{{$sheet := .Data.Sheet}} |
| +{{$mayComment := .Data.MayComment}} |
| +{{range .Data.Threads}} |
| +<div rst-box> |
| +<div rst-box-head> |
| +<strong>{{.Ref}}</strong> |
| +{{if .Resolved}}<span rst-badge="neutral">resolved</span>{{end}} |
| +</div> |
| +{{range .Entries}} |
| +<div rst-tblock> |
| +<div rst-tblock-head> |
| +<span rst-tblock-title>{{.Author}}</span> |
| +<span rst-row-sub>{{.When.Format "2 Jan 2006, 15:04"}}</span> |
| +</div> |
| +<div rst-tblock-body><p>{{.Body}}</p></div> |
| +</div> |
| +{{end}} |
| +{{if and $mayComment (not .Resolved)}} |
| +<div rst-box-foot> |
| +{{/* Resolving settles the whole cell's thread, because a thread is |
| + the unit people think in and resolving one message of a |
| + conversation says nothing useful. */}} |
| +<form method="post" action="/sheets/{{$sheet.ID}}/comments/resolve"> |
| +<input type="hidden" name="r" value="{{.RowPos}}"> |
| +<input type="hidden" name="c" value="{{.ColPos}}"> |
| +<button rst-btn="ghost" type="submit">Resolve this thread</button> |
| +</form> |
| +</div> |
| +{{end}} |
| +</div> |
| +{{end}} |
| +{{else}} |
| +<div rst-empty> |
| +<p rst-empty-title>No comments yet</p> |
| +<p rst-empty-body>A comment is a note about one cell. It stays attached to |
| +that cell even when rows are inserted above it.</p> |
| +</div> |
| +{{end}} |
| +{{end}} |
diff --git a/internal/sheets/templates/sheet.html b/internal/sheets/templates/sheet.html| index 1175dc8..4825d5a 100644 |
| --- a/internal/sheets/templates/sheet.html |
| +++ b/internal/sheets/templates/sheet.html |
| @@ -41,6 +41,7 @@ |
| <button rst-btn="ghost" type="button" data-action="fill-right" hidden>Fill right</button> |
| <a rst-btn="ghost" href="/sheets/{{.Data.Sheet.ID}}/table">Table view</a> |
| <a rst-btn="ghost" href="/sheets/{{.Data.Sheet.ID}}/history">History</a> |
| +<a rst-btn="ghost" href="/sheets/{{.Data.Sheet.ID}}/comments">Comments</a> |
| {{if .Data.MayShare}}<a rst-btn="ghost" href="/sheets/{{.Data.Sheet.ID}}/share">Share</a>{{end}} |
| <span data-sheet-live data-state="off" role="status">Connecting</span> |
| </div> |
diff --git a/internal/sheets/templates/table.html b/internal/sheets/templates/table.html| index c01cbd4..00dba93 100644 |
| --- a/internal/sheets/templates/table.html |
| +++ b/internal/sheets/templates/table.html |
| @@ -6,6 +6,7 @@ |
| Every cell is editable here, with no JavaScript at all.</p> |
| </div> |
| <a rst-btn="ghost" href="/sheets/{{.Data.Sheet.ID}}/history">History</a> |
| +<a rst-btn="ghost" href="/sheets/{{.Data.Sheet.ID}}/comments">Comments</a> |
| <a rst-btn="primary" href="/sheets/{{.Data.Sheet.ID}}">Open the grid</a> |
| </header> |