forge
ExploreLog in

Tahinli/forge

public
0 watching
★ 0
10.2 KiB
1# Milestone 4b: pull request reviews and line comments (contract between workers)
2
3The global rules from `docs/m1-interfaces.md`, `docs/m3-interfaces.md` and `docs/m4-interfaces.md` apply:
4- No JS and no inline styles.
5- Full hex OIDs only, validated paths, and user values after `--end-of-options`.
6- No unwrap in non-test code.
7- Tests use `tempfile::TempDir`.
8- Never put cargo target dirs or big files in /tmp.
9
10## Concepts
11- A **review** belongs to a PR (`issue_id`) and has an author. Its state is one of:
12 - `pending`: a draft, visible only to its author, with at most one per author per PR.
13 - `commented`, `approved` or `changes_requested`: submitted.
14 - `dismissed`: a Write holder dismissed an approval or a change request, giving a reason.
15- A **review comment** is a comment on one line of the PR diff, identified by:
16 - `path`: the new path; for a deleted file, the old path;
17 - `side`: `new` for added and context lines, `old` for deleted lines;
18 - `line`: the line number on that side;
19 - `commit_oid`: the head OID when it was written.
20 It belongs to a review. A comment whose `reply_to` is set is a reply in the root comment's thread; replies are submitted immediately, through an auto-created `commented` review without a body. A root carries `resolved`.
21- **Outdated:** after the head moves, a root comment is outdated when the blob at `path` differs between its `commit_oid` and the new head (this is coarse on purpose; line remapping comes later). Outdated threads still show in the conversation, with an "Outdated" badge, and no longer show on the Files tab.
22- `diff_hunk` stores the last up to 4 diff lines ending at the commented line, as plain text with a `+`, `-` or space prefix, captured at creation. The conversation shows it as the thread's context without running git again.
23- PR authors can't approve or request changes on their own PR; they can only comment. Anyone who can Read the base repo and is signed in can review. A review by a Read user counts in the summary, but merge rules don't enforce anything yet.
24
25## Schema (migration `0008_reviews.sql`)
26```sql
27CREATE TABLE pull_reviews (
28 id TEXT PRIMARY KEY, issue_id TEXT NOT NULL REFERENCES issues(id) ON DELETE CASCADE,
29 author_id TEXT NOT NULL REFERENCES users(id),
30 state TEXT NOT NULL, -- pending | commented | approved | changes_requested | dismissed
31 body TEXT NOT NULL DEFAULT '',
32 commit_oid TEXT NOT NULL, -- head at submit (or at creation while pending)
33 dismissed_reason TEXT,
34 created_at TEXT NOT NULL, submitted_at TEXT);
35CREATE INDEX pull_reviews_issue ON pull_reviews(issue_id, submitted_at);
36CREATE TABLE review_comments (
37 id TEXT PRIMARY KEY, issue_id TEXT NOT NULL REFERENCES issues(id) ON DELETE CASCADE,
38 review_id TEXT NOT NULL REFERENCES pull_reviews(id) ON DELETE CASCADE,
39 author_id TEXT NOT NULL REFERENCES users(id),
40 path TEXT NOT NULL, side TEXT NOT NULL, line INTEGER NOT NULL,
41 commit_oid TEXT NOT NULL, diff_hunk TEXT NOT NULL DEFAULT '',
42 reply_to TEXT REFERENCES review_comments(id) ON DELETE CASCADE,
43 body TEXT NOT NULL, outdated INTEGER NOT NULL DEFAULT 0, resolved INTEGER NOT NULL DEFAULT 0,
44 resolved_by TEXT REFERENCES users(id),
45 created_at TEXT NOT NULL, updated_at TEXT NOT NULL);
46CREATE INDEX review_comments_issue ON review_comments(issue_id, path, created_at);
47```
48Issue events gain the kinds `review_dismissed` (data `{"review","reason"}`), `thread_resolved` and `thread_unresolved`.
49
50## forge-core (`src/reviews.rs` model plus `store/reviews.rs`, an `impl Store` block)
51```rust
52pub enum ReviewState { Pending, Commented, Approved, ChangesRequested, Dismissed } // as_str / FromStr
53pub enum Side { Old, New } // "old" | "new"
54pub struct Review { pub id: Id, pub issue_id: Id, pub author_id: Id, pub author_name: String, pub state: ReviewState,
55 pub body: String, pub commit_oid: String, pub dismissed_reason: Option<String>, pub created_at: String, pub submitted_at: Option<String> }
56pub struct ReviewComment { pub id: Id, pub review_id: Id, pub author_id: Id, pub author_name: String, pub path: String, pub side: Side,
57 pub line: u32, pub commit_oid: String, pub diff_hunk: String, pub reply_to: Option<Id>, pub body: String,
58 pub outdated: bool, pub resolved: bool, pub created_at: String, pub updated_at: String, pub pending: bool /* review is pending */ }
59pub struct Thread { pub root: ReviewComment, pub replies: Vec<ReviewComment> }
60
61// store/reviews.rs
62pub async fn pending_review(&self, issue_id: &str, author_id: &str) -> Result<Option<Review>>;
63pub async fn add_review_comment(&self, issue_id: &str, author_id: &str, pending: bool, path: &str, side: Side, line: u32,
64 commit_oid: &str, diff_hunk: &str, body: &str) -> Result<ReviewComment>;
65 // pending=true: put it in the author's pending review, creating that review if needed;
66 // pending=false: create a submitted `commented` review holding just this comment
67pub async fn reply_review_comment(&self, issue_id: &str, author_id: &str, root_id: &str, body: &str) -> Result<ReviewComment>;
68 // root must be a submitted root comment of this PR; copies path/side/line/commit_oid
69pub async fn submit_review(&self, issue_id: &str, author_id: &str, state: ReviewState /* not Pending/Dismissed */,
70 body: &str, head_oid: &str) -> Result<Review>;
71 // turns the pending review into `state` (or creates one with no comments); Commented with an empty body
72 // and no comments is Invalid; the PR author's Approved or ChangesRequested is Forbidden
73pub async fn discard_pending_review(&self, issue_id: &str, author_id: &str) -> Result<()>;
74pub async fn dismiss_review(&self, review_id: &str, actor_id: &str, reason: &str) -> Result<()>; // approved or changes_requested only; adds the event
75pub async fn list_reviews(&self, issue_id: &str) -> Result<Vec<Review>>; // submitted ones, oldest first
76pub async fn review_threads(&self, issue_id: &str, viewer_id: Option<&str>) -> Result<Vec<Thread>>;
77 // submitted comments plus the viewer's own pending ones; replies oldest first
78pub async fn review_comment(&self, id: &str) -> Result<Option<ReviewComment>>;
79pub async fn edit_review_comment(&self, id: &str, body: &str) -> Result<()>; // records edit_history target_kind 'review_comment'
80pub async fn delete_review_comment(&self, id: &str) -> Result<()>; // deleting a root deletes its replies
81pub async fn set_thread_resolved(&self, root_id: &str, actor_id: &str, resolved: bool) -> Result<()>; // adds an event
82pub async fn mark_outdated(&self, issue_id: &str, ids: &[Id]) -> Result<()>;
83pub async fn review_summary(&self, issue_id: &str) -> Result<ReviewSummary>;
84 // per reviewer, their latest non-commented, non-dismissed submitted state
85pub struct ReviewSummary { pub approved_by: Vec<String>, pub changes_requested_by: Vec<String> }
86```
87Bodies use the same limits as issue comments. `author_name` comes from a join on `users`.
88
89## forge-git (added to `src/pulls.rs`)
90```rust
91pub async fn blob_at(env: &GitEnv, repo: &Path, commit: &str, path: &str) -> Result<Option<String>>; // `rev-parse <commit>:<path>` gives the blob oid; None if absent. Validate path: no NUL, not absolute, no `..` segments
92```
93
94## Outdated marking (forge-web `pulls_sync.rs`, extending the head_updated path)
95After `update_pull_head`, for each submitted root comment that isn't outdated, compare `blob_at(old commit_oid, path)` with `blob_at(new_head, path)`. Mark it outdated if they differ (cache by `(commit_oid, path)`).
96
97## Web
98- **Files tab** `/pulls/{n}/files`:
99 - **Line anchors.** Each diff line gets an id `L-<file_index>-<side>-<line>` and a small "+" link to `?comment=<file_index>:<side>:<line>#L-…`. Use `file_index` (the file's position in the diff), not the path, to keep URLs short. The server maps it back to the path and re-validates it against the current diff.
100 - **Inline comment form.** When `comment=` matches a line, render a form right under that line. It POSTs `/pulls/{n}/review_comments` with csrf, `file_index`, `side`, `line`, `head_oid` (rejected if stale: "the pull request changed, reload"), body, and two buttons: `action=single` ("Add single comment") or `action=review` ("Start a review" / "Add review comment" if one is pending).
101 - **Threads.** Existing, non-outdated threads on the current diff render under their line: comments (markdown-rendered like issue comments), a reply form, a Resolve/Unresolve button (PR author or Write), and edit/delete for the author.
102 - **Review bar** at the top for signed-in viewers: "Finish your review" with a pending comment count, a body textarea, radio comment/approve/request_changes (approve and request changes hidden for the PR author), Submit and Discard. Posts to `/pulls/{n}/reviews` or `/pulls/{n}/reviews/discard`.
103 - The `diff_section` component gains an optional slot. Implement it as an optional `&HashMap<(usize, Side, u32), Vec<...>>` of rendered thread data plus an optional open-form target; keep `commit.rs` and `compare.rs` calls working, passing none.
104- **Conversation timeline:** submitted reviews appear in created order:
105 - Avatar/name with the verb ("approved these changes" / "requested changes" / "reviewed"), the body, then its threads, each showing the `diff_hunk` as a small code block, the path:line, an "Outdated" badge, comments and replies, and a "Resolved" collapsed marker using `<details>`.
106 - Dismissed reviews show struck-through with the reason.
107- **Merge box:** a summary line: "Approved by a, b", "Changes requested by c". Write holders see a "Dismiss" form for each approve or request-changes review, with a reason.
108- **Routes** (all POST with CSRF, all redirecting 303 back to the referring tab with an anchor):
109 - `/pulls/{n}/review_comments`
110 - `/pulls/{n}/review_comments/{id}/reply`
111 - `/pulls/{n}/review_comments/{id}/edit` (GET form + POST)
112 - `/pulls/{n}/review_comments/{id}/delete`
113 - `/pulls/{n}/review_comments/{id}/resolve` (with `resolved=1|0`)
114 - `/pulls/{n}/reviews`, `/pulls/{n}/reviews/discard`, `/pulls/{n}/reviews/{id}/dismiss`
115- **Permissions:**
116 - Read and signed in to comment or review.
117 - Author of the comment to edit or delete; Triage+ may also delete.
118 - PR author or Write to resolve; Write to dismiss.
119 - Archived repo: no writes. Merged or closed PR: comments and replies still allowed, new reviews not.