From 7b84196526e001ea4ef8f240fdc5ef10bd2f89c0 Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Sun, 30 Aug 2026 13:29:53 -0700 Subject: [PATCH] review: Accept file and range drafts Review comment anchors accept a file, a postimage line, or an inclusive postimage line range. Immediate comments already preserve all three scopes, but local drafts retain only one line. Persist and display each draft's complete anchor. Keep the existing `line` field compatible with single-line records, and submit the original scope when the draft is published. --- internal/handler/review/comment.go | 4 -- internal/handler/review/publish.go | 9 ++- internal/spice/state/review_draft.go | 8 ++- internal/spice/state/review_draft_test.go | 46 +++++++++++++ review_list.go | 2 + testdata/script/review_draft_scopes.txt | 82 +++++++++++++++++++++++ testdata/script/review_list_json.txt | 2 +- 7 files changed, 146 insertions(+), 7 deletions(-) create mode 100644 testdata/script/review_draft_scopes.txt diff --git a/internal/handler/review/comment.go b/internal/handler/review/comment.go index 7f5ebe72..c6a5fb3c 100644 --- a/internal/handler/review/comment.go +++ b/internal/handler/review/comment.go @@ -29,10 +29,6 @@ func (h *DraftHandler) SaveCommentDraft( ctx context.Context, req *CommentRequest, ) error { - if !req.Anchor.IsLine() { - return errors.New("draft comments require a single-line file:line anchor") - } - body, err := h.commentBody(ctx, req.Message) if err != nil { return err diff --git a/internal/handler/review/publish.go b/internal/handler/review/publish.go index 74e9a0ce..fc1b389f 100644 --- a/internal/handler/review/publish.go +++ b/internal/handler/review/publish.go @@ -74,7 +74,14 @@ func (h *Handler) PublishDrafts( continue } - if !patch.ContainsLineRange( + if draft.Anchor.IsFile() && !patch.ContainsFile(draft.Anchor.Path) { + return fmt.Errorf( + "draft %s: review diff does not contain file %q", + draft.ID, + draft.Anchor.Path, + ) + } + if !draft.Anchor.IsFile() && !patch.ContainsLineRange( draft.Anchor.Path, draft.Anchor.StartLine, draft.Anchor.EndLine, diff --git a/internal/spice/state/review_draft.go b/internal/spice/state/review_draft.go index 700c1025..1c96669d 100644 --- a/internal/spice/state/review_draft.go +++ b/internal/spice/state/review_draft.go @@ -25,6 +25,7 @@ type reviewDraftState struct { type storedReviewDraft struct { File string `json:"file"` Line int `json:"line"` + EndLine int `json:"endLine,omitempty"` Body string `json:"body"` ThreadID string `json:"threadID,omitempty"` } @@ -220,13 +221,17 @@ func (s *Store) LoadReviewDrafts( continue } + endLine := stored.EndLine + if stored.Line != 0 && endLine == 0 { + endLine = stored.Line + } drafts[i] = review.Draft{ ID: id, Body: stored.Body, Anchor: review.Anchor{ Path: stored.File, StartLine: stored.Line, - EndLine: stored.Line, + EndLine: endLine, }, } } @@ -258,6 +263,7 @@ func storeReviewDraft(draft review.Draft) storedReviewDraft { stored.File = draft.Anchor.Path stored.Line = draft.Anchor.StartLine + stored.EndLine = draft.Anchor.EndLine return stored } diff --git a/internal/spice/state/review_draft_test.go b/internal/spice/state/review_draft_test.go index 30f46787..349e8804 100644 --- a/internal/spice/state/review_draft_test.go +++ b/internal/spice/state/review_draft_test.go @@ -222,3 +222,49 @@ func TestReviewDraftsFollowBranchLifecycle(t *testing.T) { assert.Nil(t, drafts) }) } + +func TestReviewDraftScopes(t *testing.T) { + t.Parallel() + + ctx := t.Context() + store, err := state.InitStore(ctx, state.InitStoreRequest{ + DB: storage.NewDB(make(storage.MapBackend)), + Trunk: "main", + }) + require.NoError(t, err) + tx := store.BeginBranchTx() + require.NoError(t, tx.Upsert(ctx, state.UpsertRequest{ + Name: "feature", + Base: "main", + })) + require.NoError(t, tx.Commit(ctx, "track feature")) + + fileDraft, err := store.AddReviewDraft( + ctx, + "feature", + review.Draft{ + ID: 0, + Body: "file body", + Anchor: review.Anchor{Path: "main.go"}, + }, + ) + require.NoError(t, err) + rangeDraft, err := store.AddReviewDraft( + ctx, + "feature", + review.Draft{ + ID: 0, + Body: "range body", + Anchor: review.Anchor{ + Path: "main.go", + StartLine: 2, + EndLine: 4, + }, + }, + ) + require.NoError(t, err) + + drafts, err := store.LoadReviewDrafts(ctx, "feature") + require.NoError(t, err) + assert.Equal(t, []review.Draft{fileDraft, rangeDraft}, drafts) +} diff --git a/review_list.go b/review_list.go index e7387d59..2d5315ea 100644 --- a/review_list.go +++ b/review_list.go @@ -11,6 +11,7 @@ import ( "time" "github.com/alecthomas/kong" + "go.abhg.dev/gs/internal/forge" "go.abhg.dev/gs/internal/git" "go.abhg.dev/gs/internal/handler/review" "go.abhg.dev/gs/internal/silog" @@ -193,6 +194,7 @@ func reviewDraftToJSON(draft review.Draft) jsonComment { comment.Scope = "file" } else { comment.Scope = "line" + comment.Side = forge.ReviewThreadSideRight.String() if !draft.Anchor.IsLine() { comment.Range = &jsonCommentRange{ Start: draft.Anchor.StartLine, diff --git a/testdata/script/review_draft_scopes.txt b/testdata/script/review_draft_scopes.txt new file mode 100644 index 00000000..8036e900 --- /dev/null +++ b/testdata/script/review_draft_scopes.txt @@ -0,0 +1,82 @@ +# Draft and publish file-level and line-range review comments. + +as 'Test ' +at '2024-04-05T16:40:32Z' + +cd repo +git init +git commit --allow-empty -m 'Initial commit' + +gs repo init +shamhub-setup +shamhub new origin alice/example.git +shamhub register alice +git push origin main + +env SHAMHUB_USERNAME=alice +gs auth login + +git add main.go +gs bc -m 'Add main' feature1 +gs branch submit --fill +stderr 'Created #' + +# Draft the same file and range anchors accepted by immediate comments. +gs review comment main.go:4-5 -m 'Range draft.' +stderr 'Drafted comment 1 on main.go:4-5' +gs review comment main.go -m 'File draft.' +stderr 'Drafted comment 2 on main.go' + +# Text and JSON output preserve each draft's complete anchor. +gs review list --draft-only +stderr 'main\.go:4-5' +stderr 'main\.go$' +gs review list --draft-only --json +cmp stdout $WORK/golden/drafts.ndjson + +# Publishing retains the file and inclusive range scopes. +gs review publish +stderr 'Published 2 comment' +shamhub dump reviews 1 +cmp stdout $WORK/golden/reviews.yaml + +-- repo/main.go -- +package main + +func main() { + println("hello") + println("world") +} +-- golden/drafts.ndjson -- +{"kind":"draft","id":"1","scope":"line","path":"main.go","line":4,"range":{"start":4,"end":5},"side":"right","body":"Range draft."} +{"kind":"draft","id":"2","scope":"file","path":"main.go","body":"File draft."} +-- golden/reviews.yaml -- +changes: + - change: 1 + submissions: + - submitter: alice + disposition: comment + commentIDs: + - 2 + - 3 + threads: + - id: thread-2 + path: main.go + range: + start: 4 + end: 5 + side: right + resolved: false + outdated: false + comments: + - id: 2 + author: alice + body: Range draft. + - id: thread-3 + path: main.go + resolved: false + outdated: false + comments: + - id: 3 + author: alice + body: File draft. diff --git a/testdata/script/review_list_json.txt b/testdata/script/review_list_json.txt index 4dbb6e49..b39ed1ed 100644 --- a/testdata/script/review_list_json.txt +++ b/testdata/script/review_list_json.txt @@ -61,4 +61,4 @@ func main() { } -- golden/draft_only.json -- -{"kind":"draft","id":"1","scope":"line","path":"main.go","line":5,"body":"Add error handling here."} +{"kind":"draft","id":"1","scope":"line","path":"main.go","line":5,"side":"right","body":"Add error handling here."}