~bigbes/sr-ht-compare

ref: f8283861787a361d6fa8bc6467ef15c9495b8261 sr-ht-compare/docs/inline-comments.md -rw-r--r-- 11.8 KiB
f8283861 — bigbes ci: a test task, and the truth about !check 9 days ago

#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

type DiffLineAnnotation<T> = { 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.

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 — login.FromContext(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:

[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):

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), sr-ht-ecore/login (Optional, FromContext), 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.