From 4a74121628f08d8203a9a6ec5d18318b2b7de91d Mon Sep 17 00:00:00 2001 From: bigbes Date: Sat, 18 Jul 2026 23:45:45 +0300 Subject: [PATCH] docs: spec for inline code comments on diffs Draft proposal for line-anchored comment threads on the commit and compare pages. Records the key finding that storage is already available through sr-ht-core (Postgres via the database package + connection-string), that the service is stateless by choice rather than by limitation, and that @pierre/diffs already provides the thread UI primitives (DiffLineAnnotation + renderAnnotation + onDiffLineClick). Covers the anchoring model, schema, authz reuse, HTTP API, frontend wiring, config, and a commit-first phasing. --- docs/inline-comments.md | 256 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 256 insertions(+) create mode 100644 docs/inline-comments.md diff --git a/docs/inline-comments.md b/docs/inline-comments.md new file mode 100644 index 0000000000000000000000000000000000000000..5cbad38e5ac4f89d2252ebb1d50508bb073507c7 --- /dev/null +++ b/docs/inline-comments.md @@ -0,0 +1,256 @@ +# Spec: inline code comments on diffs + +Status: **draft / proposal** — not yet implemented. +Author: bigbes. Last updated: 2026-07-18. + +## 1. Motivation + +The reference diff viewers (GitHub, the `diffshub` design we mirror) let a viewer +attach a comment to a specific line of a diff and reply in a thread. compare.sr.ht +renders commit and compare (`base...head`) diffs but has no way to discuss them +in place. SourceHut's native review flow is patch-over-mailing-list +(`lists.sr.ht`); inline comments are a **net-new capability** for the instance, +not something we can federate from an existing service. + +This document specifies adding line-anchored comment threads to the commit and +compare pages. + +## 2. Key constraint: the service is stateless *by choice* + +compare.sr.ht owns no persistence today. `cmd/comparesrht/main.go` spells it out: + +> it deliberately does NOT call `WithDefaultMiddleware`: compare.sr.ht owns no +> Postgres or Redis and must serve anonymous viewers + +Crucially this is a **choice, not a missing capability**. The shared +`sr-ht-core` library already ships the storage layer the rest of the fleet uses, +and it is already in our build (as an indirect dependency: `github.com/lib/pq`): + +- `sr-ht-core/database` — connection-pool context middleware + (`Middleware`, `DBForContext`, `ForContext`), transaction helpers + (`WithTx`, `WithReadOnlyTx`), and a squirrel-based query builder with a + `Model`/`Scan` layer (`sq.go`, `ql.go`). +- `sr-ht-core/server` `WithDefaultMiddleware()` opens the pool from a + `[compare.sr.ht]::connection-string` key via `sql.Open("postgres", …)`. + +So the storage foundation is sitting unused. Adding comments means **turning that +path back on** for the comment routes while keeping anonymous, DB-free browsing +of diffs. The datastore is **Postgres** (the instance already runs it for +`git.sr.ht`/`meta.sr.ht`) — not SQLite. + +### What flipping storage on costs us + +Adding a DB makes the service no longer stateless. Downstream updates required: + +- `README.md` and the `server.go` package doc ("owns no state of its own"). +- The `main.go` "no database" comment and middleware group. +- `contrib/compare-srht.service` — add a Postgres ordering dependency. +- Ops: the service now has backup/restore concerns and a schema to migrate. + +## 3. Anchoring model (the crux) + +A comment must pin to a place in a diff and survive re-rendering. `@pierre/diffs` +gives us the exact primitive: a `DiffLineAnnotation` is anchored by + +```ts +type DiffLineAnnotation = { side: 'additions' | 'deletions'; lineNumber: number } & { metadata?: T } +``` + +So the natural per-file anchor is **`(file_path, side, line_number)`**, where +`side` selects the new-file column (`additions`) or old-file column +(`deletions`) and `line_number` is 1-based within that side. That maps 1:1 to a +DB row and to the widget the library renders. + +The harder question is *which diff* the anchor lives in: + +- **Commit page** (`/commit/{rev}`): the diff is immutable — a fixed commit SHA + vs. its first parent. Anchor = `(commit_sha, file_path, side, line_number)`. + Stable forever. **This is the easy, correct MVP surface.** +- **Compare page** (`/compare/{base}...{head}`): `base`/`head` are usually + branch refs that *move*. Anchoring to symbolic ref names is fragile (a push + silently re-targets the comment). Resolve refs to SHAs at comment time and + anchor to `(base_sha, head_sha, three_dot, file, side, line)`. When the refs + later move, show the thread as **outdated** (like GitHub) rather than + silently re-hanging it on unrelated lines. + +**Recommendation:** ship commit-page comments first (immutable anchor, no +outdated-state machine), add compare-range comments in a second phase. + +## 4. Data model + +One table, self-threading via `thread_root`. + +```sql +CREATE TABLE comment ( + id BIGSERIAL PRIMARY KEY, + created TIMESTAMPTZ NOT NULL DEFAULT now(), + updated TIMESTAMPTZ NOT NULL DEFAULT now(), + author TEXT NOT NULL, -- meta.sr.ht canonical username + + -- repository (from authz.RepoInfo + route params) + repo_id INTEGER NOT NULL, -- git.sr.ht repository id + owner TEXT NOT NULL, -- ~owner (for routing / listing) + repo_name TEXT NOT NULL, + + -- anchor + target_kind TEXT NOT NULL, -- 'commit' | 'compare' + commit_sha TEXT, -- target_kind='commit' + base_sha TEXT, -- target_kind='compare' + head_sha TEXT, + three_dot BOOLEAN, -- compare: '...' vs '..' + file_path TEXT NOT NULL, -- repo path, no a//b/ prefix + side TEXT NOT NULL, -- 'additions' | 'deletions' + line_number INTEGER NOT NULL, -- 1-based, within side + + -- threading + content + thread_root BIGINT REFERENCES comment(id) ON DELETE CASCADE, -- NULL => root + body TEXT NOT NULL, -- markdown source + resolved BOOLEAN NOT NULL DEFAULT false +); + +CREATE INDEX comment_commit_idx ON comment (repo_id, commit_sha); +CREATE INDEX comment_compare_idx ON comment (repo_id, base_sha, head_sha); +``` + +A **thread** is a root comment (`thread_root IS NULL`) plus its replies. A root +carries the anchor; replies inherit it (denormalize the anchor onto replies too, +or read it from the root — denormalizing keeps list queries single-table). +`resolved` is a thread-level flag stored on the root. + +## 5. Authorization + +Reuse the existing seams — do **not** invent a second authz path. + +- **Read**: identical to page authz. Every handler already calls `s.resolve()` + (`authz.Authorizer.Repo`), which 404s a repo the viewer cannot see (private + existence never leaks). Comments for a repo are only ever returned to a viewer + who passed that check. +- **Write**: require an authenticated viewer — `authz.ForContext(ctx) != ""`. + For the MVP, **any authenticated viewer who can read the repo may comment** + (open code review). Edit/delete restricted to the comment's `author`; resolve + allowed to the thread author or comment author. +- **Repo-member-only gating (optional, later)**: `authz.RepoInfo` currently + carries no ACL/write-access (`ID, Name, Description, Visibility` only). To gate + writes to repo members, extend the GraphQL query in `authz/authz.go` to fetch + the viewer's access level and add it to `RepoInfo`. + +**CSRF**: writes are cookie-authenticated (the unified-login cookie), so mutation +endpoints need CSRF protection — issue a per-session token into the page and +require it on POST/PATCH/DELETE (or enforce a custom header + `SameSite` cookie). + +## 6. HTTP API + +REST/JSON, mounted alongside the current routes in `web/router.go`. All routes +authorize via `s.resolve()` first; mutations additionally require a logged-in +viewer and a valid CSRF token. + +``` +GET /~{owner}/{repo}/commit/{rev}/comments list threads for a commit +POST /~{owner}/{repo}/commit/{rev}/comments create root comment or reply +GET /~{owner}/{repo}/compare/{spec}/comments list threads for a compare (phase 2) +POST /~{owner}/{repo}/compare/{spec}/comments create (phase 2) +PATCH /~{owner}/{repo}/comments/{id} edit body / toggle resolved +DELETE /~{owner}/{repo}/comments/{id} delete (author only) +``` + +`POST` body: `{ file_path, side, line_number, body, thread_root? }`. + +### Initial load: embed, don't round-trip + +The page is already SSR + one embedded JSON blob (`#compare-data`) and the design +ethos (see `web/server.go`) is "single request, no second authorization +round-trip." Follow it: the commit/compare handlers query the DB and include the +threads in the existing `compareData` payload, so the first paint already shows +comments. The `GET …/comments` endpoint exists for re-fetch after a mutation; the +`POST/PATCH/DELETE` endpoints drive mutations (optimistic update, then reconcile). + +## 7. Frontend integration (`frontend/src/app.ts`) + +`@pierre/diffs` `FileDiff` already supports comment threads as first-class UI — +no new rendering library: + +1. **Render existing threads.** Build `DiffLineAnnotation[]` from the embedded + threads (`{ side, lineNumber, metadata: thread }`) and pass them as + `lineAnnotations` to `FileDiff.render()`. +2. **Render the widget.** Provide `renderAnnotation(annotation)` in + `FileDiffOptions` returning the thread DOM (comment list + reply box + + resolve/edit/delete controls). +3. **Start a new thread.** Set `onDiffLineClick` (from + `InteractionManagerBaseOptions<'diff'>`) — its props give `{ side, lineNumber }` + — to inject a transient "new comment" annotation with a composer. +4. **Persist.** On submit, `POST` to the API; on success, replace the transient + annotation with the saved thread and `rerender()` the instance. The layout + toggle path (`wireLayoutToggle`) must preserve annotations across split↔unified + re-renders. + +Comment bodies are Markdown → **sanitize** on render (or render server-side to +sanitized HTML). Never inject raw comment HTML. + +## 8. Config & wiring + +New key: + +```ini +[compare.sr.ht] +connection-string=postgres://compare:secret@localhost/compare?sslmode=disable +``` + +In `cmd/comparesrht/main.go`, open the pool once at startup and add +`database.Middleware(db)` to the existing anonymous `Group` (keeping anonymous +browsing intact — the middleware only injects the pool; only comment handlers +touch it): + +```go +db, err := sql.Open("postgres", connString) // from [compare.sr.ht] connection-string +// … +r.Use(database.Middleware(db)) // added alongside config + authz middleware +``` + +Handlers use `database.WithReadOnlyTx` / `WithTx` + the `sq` query builder. Add +`connection-string` to `validateConfig`'s required-key set so a misconfiguration +fails loudly at startup, consistent with the existing validation. + +## 9. Schema management + +`sr-ht-core/database` provides the connection and query layers but **no migration +runner**. Options, cheapest first: + +- Ship `schema.sql` and a `migrations/` dir; apply with a `comparesrht -migrate` + subcommand run on deploy (self-contained, no new dependency). +- Adopt a Go migration library (e.g. `golang-migrate`) if versioned migrations + become worth the dependency. + +Confirm how the sibling Go services on this instance manage schema and match them. + +## 10. Out of scope (for now) + +- Email / webhook notifications on new comments (natural follow-up; `sr-ht-core` + has `email` and `webhooks` packages if we want them later). +- Reactions, attachments, suggested-edit ("commit suggestion") blocks. +- Cross-diff thread migration on rebase beyond the "outdated" marker. + +## 11. Phasing + +1. **Phase 1 — commit comments.** Schema + DB wiring + immutable-anchor threads + on `/commit/{rev}`; threads embedded in the page JSON; `POST/PATCH/DELETE` + REST; `renderAnnotation` + `onDiffLineClick` in the bundle. Auth: any + authenticated viewer who can read the repo. Edit/delete: author only. +2. **Phase 2 — compare comments.** SHA-pinned anchors on + `/compare/{base}...{head}`, with an **outdated** state when refs move. +3. **Phase 3 — polish.** Resolve/unresolve UX, Markdown rendering, notifications, + optional repo-member-only gating (extend `RepoInfo` with access level). + +## Appendix: grounding references + +- Storage layer: `sr-ht-core/database/{middleware,sq,ql}.go`; + pool wiring `sr-ht-core/server/server.go` (`WithDefaultMiddleware`, + `connection-string`). +- Opt-out today: `cmd/comparesrht/main.go` (middleware `Group`, `validateConfig`). +- Authz seams: `authz/authz.go` (`Authorizer`, `RepoInfo`), + `authz/identity.go` (`ForContext`), `web/handlers.go` (`s.resolve`). +- Routes / SSR contract: `web/router.go` (`Register`), `web/server.go`, + `web/handlers.go` (`buildCompareJSON`, `#compare-data`). +- Diff library API: `@pierre/diffs` `FileDiffOptions.renderAnnotation`, + `FileDiffRenderProps.lineAnnotations`, `DiffLineAnnotation` (`side` + + `lineNumber`), `InteractionManager` `onDiffLineClick` / + `OnDiffLineClickProps`.