diff --git a/internal/handler/review/handler.go b/internal/handler/review/handler.go index e8a295d0..0094949f 100644 --- a/internal/handler/review/handler.go +++ b/internal/handler/review/handler.go @@ -65,8 +65,8 @@ var _ Service = (*spice.Service)(nil) type Store interface { AddReviewDraft(context.Context, string, review.Draft) (review.Draft, error) LoadReviewDrafts(context.Context, string) ([]review.Draft, error) + RemovePublishedReviewDrafts(context.Context, string, []review.Draft) error UpdateReviewDraftBody(context.Context, string, review.DraftID, string) error - ClearReviewDrafts(context.Context, string) error } var _ Store = (*state.Store)(nil) diff --git a/internal/handler/review/handler_test.go b/internal/handler/review/handler_test.go index f608e963..9fc7d3f8 100644 --- a/internal/handler/review/handler_test.go +++ b/internal/handler/review/handler_test.go @@ -356,7 +356,7 @@ func TestHandler_PublishDrafts(t *testing.T) { Return(forge.SubmitReviewResult{}, nil) store. EXPECT(). - ClearReviewDrafts(gomock.Any(), "feature"). + RemovePublishedReviewDrafts(gomock.Any(), "feature", drafts). Return(nil) err := handler.PublishDrafts(t.Context(), &PublishDraftsRequest{ diff --git a/internal/handler/review/mocks_test.go b/internal/handler/review/mocks_test.go index d6751ca4..5f9dff06 100644 --- a/internal/handler/review/mocks_test.go +++ b/internal/handler/review/mocks_test.go @@ -208,79 +208,79 @@ func (c *MockStoreAddReviewDraftCall) DoAndReturn(f func(context.Context, string return c } -// ClearReviewDrafts mocks base method. -func (m *MockStore) ClearReviewDrafts(arg0 context.Context, arg1 string) error { +// LoadReviewDrafts mocks base method. +func (m *MockStore) LoadReviewDrafts(arg0 context.Context, arg1 string) ([]review.Draft, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "ClearReviewDrafts", arg0, arg1) - ret0, _ := ret[0].(error) - return ret0 + ret := m.ctrl.Call(m, "LoadReviewDrafts", arg0, arg1) + ret0, _ := ret[0].([]review.Draft) + ret1, _ := ret[1].(error) + return ret0, ret1 } -// ClearReviewDrafts indicates an expected call of ClearReviewDrafts. -func (mr *MockStoreMockRecorder) ClearReviewDrafts(arg0, arg1 any) *MockStoreClearReviewDraftsCall { +// LoadReviewDrafts indicates an expected call of LoadReviewDrafts. +func (mr *MockStoreMockRecorder) LoadReviewDrafts(arg0, arg1 any) *MockStoreLoadReviewDraftsCall { mr.mock.ctrl.T.Helper() - call := mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ClearReviewDrafts", reflect.TypeOf((*MockStore)(nil).ClearReviewDrafts), arg0, arg1) - return &MockStoreClearReviewDraftsCall{Call: call} + call := mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "LoadReviewDrafts", reflect.TypeOf((*MockStore)(nil).LoadReviewDrafts), arg0, arg1) + return &MockStoreLoadReviewDraftsCall{Call: call} } -// MockStoreClearReviewDraftsCall wrap *gomock.Call -type MockStoreClearReviewDraftsCall struct { +// MockStoreLoadReviewDraftsCall wrap *gomock.Call +type MockStoreLoadReviewDraftsCall struct { *gomock.Call } // Return rewrite *gomock.Call.Return -func (c *MockStoreClearReviewDraftsCall) Return(arg0 error) *MockStoreClearReviewDraftsCall { - c.Call = c.Call.Return(arg0) +func (c *MockStoreLoadReviewDraftsCall) Return(arg0 []review.Draft, arg1 error) *MockStoreLoadReviewDraftsCall { + c.Call = c.Call.Return(arg0, arg1) return c } // Do rewrite *gomock.Call.Do -func (c *MockStoreClearReviewDraftsCall) Do(f func(context.Context, string) error) *MockStoreClearReviewDraftsCall { +func (c *MockStoreLoadReviewDraftsCall) Do(f func(context.Context, string) ([]review.Draft, error)) *MockStoreLoadReviewDraftsCall { c.Call = c.Call.Do(f) return c } // DoAndReturn rewrite *gomock.Call.DoAndReturn -func (c *MockStoreClearReviewDraftsCall) DoAndReturn(f func(context.Context, string) error) *MockStoreClearReviewDraftsCall { +func (c *MockStoreLoadReviewDraftsCall) DoAndReturn(f func(context.Context, string) ([]review.Draft, error)) *MockStoreLoadReviewDraftsCall { c.Call = c.Call.DoAndReturn(f) return c } -// LoadReviewDrafts mocks base method. -func (m *MockStore) LoadReviewDrafts(arg0 context.Context, arg1 string) ([]review.Draft, error) { +// RemovePublishedReviewDrafts mocks base method. +func (m *MockStore) RemovePublishedReviewDrafts(arg0 context.Context, arg1 string, arg2 []review.Draft) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "LoadReviewDrafts", arg0, arg1) - ret0, _ := ret[0].([]review.Draft) - ret1, _ := ret[1].(error) - return ret0, ret1 + ret := m.ctrl.Call(m, "RemovePublishedReviewDrafts", arg0, arg1, arg2) + ret0, _ := ret[0].(error) + return ret0 } -// LoadReviewDrafts indicates an expected call of LoadReviewDrafts. -func (mr *MockStoreMockRecorder) LoadReviewDrafts(arg0, arg1 any) *MockStoreLoadReviewDraftsCall { +// RemovePublishedReviewDrafts indicates an expected call of RemovePublishedReviewDrafts. +func (mr *MockStoreMockRecorder) RemovePublishedReviewDrafts(arg0, arg1, arg2 any) *MockStoreRemovePublishedReviewDraftsCall { mr.mock.ctrl.T.Helper() - call := mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "LoadReviewDrafts", reflect.TypeOf((*MockStore)(nil).LoadReviewDrafts), arg0, arg1) - return &MockStoreLoadReviewDraftsCall{Call: call} + call := mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RemovePublishedReviewDrafts", reflect.TypeOf((*MockStore)(nil).RemovePublishedReviewDrafts), arg0, arg1, arg2) + return &MockStoreRemovePublishedReviewDraftsCall{Call: call} } -// MockStoreLoadReviewDraftsCall wrap *gomock.Call -type MockStoreLoadReviewDraftsCall struct { +// MockStoreRemovePublishedReviewDraftsCall wrap *gomock.Call +type MockStoreRemovePublishedReviewDraftsCall struct { *gomock.Call } // Return rewrite *gomock.Call.Return -func (c *MockStoreLoadReviewDraftsCall) Return(arg0 []review.Draft, arg1 error) *MockStoreLoadReviewDraftsCall { - c.Call = c.Call.Return(arg0, arg1) +func (c *MockStoreRemovePublishedReviewDraftsCall) Return(arg0 error) *MockStoreRemovePublishedReviewDraftsCall { + c.Call = c.Call.Return(arg0) return c } // Do rewrite *gomock.Call.Do -func (c *MockStoreLoadReviewDraftsCall) Do(f func(context.Context, string) ([]review.Draft, error)) *MockStoreLoadReviewDraftsCall { +func (c *MockStoreRemovePublishedReviewDraftsCall) Do(f func(context.Context, string, []review.Draft) error) *MockStoreRemovePublishedReviewDraftsCall { c.Call = c.Call.Do(f) return c } // DoAndReturn rewrite *gomock.Call.DoAndReturn -func (c *MockStoreLoadReviewDraftsCall) DoAndReturn(f func(context.Context, string) ([]review.Draft, error)) *MockStoreLoadReviewDraftsCall { +func (c *MockStoreRemovePublishedReviewDraftsCall) DoAndReturn(f func(context.Context, string, []review.Draft) error) *MockStoreRemovePublishedReviewDraftsCall { c.Call = c.Call.DoAndReturn(f) return c } diff --git a/internal/handler/review/publish.go b/internal/handler/review/publish.go index bffa03b8..74e9a0ce 100644 --- a/internal/handler/review/publish.go +++ b/internal/handler/review/publish.go @@ -107,8 +107,12 @@ func (h *Handler) PublishDrafts( ); err != nil { return fmt.Errorf("submit review: %w", err) } - if err := h.Store.ClearReviewDrafts(ctx, req.Branch); err != nil { - return fmt.Errorf("clear draft comments: %w", err) + if err := h.Store.RemovePublishedReviewDrafts( + ctx, + req.Branch, + drafts, + ); err != nil { + return fmt.Errorf("remove published draft comments: %w", err) } h.Log.Infof( diff --git a/internal/jsonmut/jsonmut.go b/internal/jsonmut/jsonmut.go index ad446f1a..78684925 100644 --- a/internal/jsonmut/jsonmut.go +++ b/internal/jsonmut/jsonmut.go @@ -125,6 +125,8 @@ func Block(statements ...Statement) Statement { for i, statement := range statements { must.Bef(statement.apply != nil, "statement %d is a zero Statement", i) } + // A program may outlive the slice used to construct it. + // Keep its replay sequence independent of later caller mutations. statements = slices.Clone(statements) return Statement{ apply: func(document jsontext.Value) (jsontext.Value, struct{}, error) { @@ -182,67 +184,82 @@ func Decode[T any](path jsontext.Pointer) Program[T] { // Set returns a statement that adds or replaces the value at path. // Parent objects addressed by path must exist. func Set(path jsontext.Pointer, value jsontext.Value) Statement { - return newSetStatement(path, value, setUpsert) + return newMutationStatement(path, value, mutationUpsert) } // SetIfAbsent returns a statement that adds value only when path is absent. // Parent objects addressed by path must exist. func SetIfAbsent(path jsontext.Pointer, value jsontext.Value) Statement { - return newSetStatement(path, value, setIfAbsent) + return newMutationStatement(path, value, mutationIfAbsent) } // Insert returns a statement that adds value at path. // It returns [ErrExist] when path already exists. // Parent objects addressed by path must exist. func Insert(path jsontext.Pointer, value jsontext.Value) Statement { - return newSetStatement(path, value, setInsert) + return newMutationStatement(path, value, mutationInsert) } // Replace returns a statement that replaces the value at path. // It returns [ErrNotExist] when path does not exist. func Replace(path jsontext.Pointer, value jsontext.Value) Statement { - return newSetStatement(path, value, setReplace) + return newMutationStatement(path, value, mutationReplace) } -type setMode uint8 +// Delete returns a statement that removes the object member at path. +// It returns [ErrNotExist] when path does not exist. +func Delete(path jsontext.Pointer) Statement { + must.Bef(path.IsValid(), "invalid JSON pointer %q", path) + must.Bef(path != "", "delete path must not be the document root") + return newMutationStatement(path, nil, mutationDelete) +} + +type mutationMode uint8 const ( - setUpsert setMode = iota - setIfAbsent - setInsert - setReplace + mutationUpsert mutationMode = iota + mutationIfAbsent + mutationInsert + mutationReplace + mutationDelete ) -func newSetStatement( +func newMutationStatement( path jsontext.Pointer, value jsontext.Value, - mode setMode, + mode mutationMode, ) Statement { must.Bef(path.IsValid(), "invalid JSON pointer %q", path) - must.Bef(value.IsValid(), "invalid JSON mutation value") - value = value.Clone() + if mode != mutationDelete { + must.Bef(value.IsValid(), "invalid JSON mutation value") + // Programs may run after the caller reuses value's backing buffer. + // Capture immutable input so every replay applies the same mutation. + value = value.Clone() + } return Statement{ apply: func(document jsontext.Value) (jsontext.Value, struct{}, error) { - updated, err := applySet(document, path, value, mode) + updated, err := applyMutation(document, path, value, mode) return updated, struct{}{}, err }, } } -func applySet( +func applyMutation( document jsontext.Value, pointer jsontext.Pointer, replacement jsontext.Value, - mode setMode, + mode mutationMode, ) (jsontext.Value, error) { operation := "set" switch mode { - case setIfAbsent: + case mutationIfAbsent: operation = "set if absent" - case setInsert: + case mutationInsert: operation = "insert" - case setReplace: + case mutationReplace: operation = "replace" + case mutationDelete: + operation = "delete" } next, stop := iter.Pull(pointer.Tokens()) @@ -270,20 +287,24 @@ func rewriteAtPath( document jsontext.Value, next func() (string, bool), replacement jsontext.Value, - mode setMode, + mode mutationMode, ) (updated jsontext.Value, changed bool, _ error) { member, ok := next() if !ok { + // An empty pointer selects the complete document, + // so no object traversal or ancestor reconstruction is necessary. switch mode { - case setInsert: + case mutationInsert: return nil, false, ErrExist - case setIfAbsent: + case mutationIfAbsent: return document, false, nil default: return replacement.Clone(), true, nil } } + // SetIfAbsent returns the complete input when the target already exists, + // even though traversal may have started encoding object prefixes. original := document var ancestors []*objectRewriteFrame @@ -309,6 +330,12 @@ func rewriteAtPath( if !found { return nil, false, ErrNotExist } + // advanceTo leaves the selected name unwritten. + // This member is an ancestor, so preserve its name now. + // Unwinding supplies its transformed child value. + if err := frame.encoder.WriteToken(jsontext.String(member)); err != nil { + return nil, false, fmt.Errorf("write member %q: %w", member, err) + } child, err := frame.decoder.ReadValue() if err != nil { @@ -329,7 +356,7 @@ func rewriteAtPath( return nil, false, err } if !found { - if mode == setReplace { + if mode == mutationReplace || mode == mutationDelete { return nil, false, ErrNotExist } if err := frame.encoder.WriteToken(jsontext.String(member)); err != nil { @@ -340,16 +367,24 @@ func rewriteAtPath( } } else { switch mode { - case setInsert: + case mutationInsert: return nil, false, ErrExist - case setIfAbsent: + case mutationIfAbsent: return original, false, nil } + // advanceTo left the selected name unwritten. + // Replacement modes emit the name and new value; + // deletion emits neither part of the member. if err := frame.decoder.SkipValue(); err != nil { return nil, false, fmt.Errorf("skip member %q: %w", member, err) } - if err := frame.encoder.WriteValue(replacement); err != nil { - return nil, false, fmt.Errorf("write member %q: %w", member, err) + if mode != mutationDelete { + if err := frame.encoder.WriteToken(jsontext.String(member)); err != nil { + return nil, false, fmt.Errorf("write member %q: %w", member, err) + } + if err := frame.encoder.WriteValue(replacement); err != nil { + return nil, false, fmt.Errorf("write member %q: %w", member, err) + } } } updated, err = frame.finish() @@ -405,34 +440,41 @@ func newObjectRewriteFrame(document jsontext.Value) (*objectRewriteFrame, error) } // advanceTo copies complete object members until member is found. -// When it succeeds, it has copied the member name but leaves its value unread. +// When it succeeds, it leaves both the member name and value unwritten, +// with the decoder positioned before the value. func (f *objectRewriteFrame) advanceTo(member string) (bool, error) { for f.decoder.PeekKind() != '}' { name, err := f.decoder.ReadToken() if err != nil { return false, fmt.Errorf("read member name: %w", err) } + nameString := name.String() + if nameString == member { + // Leave the selected name out of the encoded prefix. + // The caller decides whether to preserve it with a transformed value + // or omit the complete member for deletion. + return true, nil + } if err := f.encoder.WriteToken(name); err != nil { return false, fmt.Errorf("write member name: %w", err) } - if name.String() == member { - return true, nil - } // Read and write each unrelated value before the decoder advances // and invalidates the raw JSON returned by ReadValue. value, err := f.decoder.ReadValue() if err != nil { - return false, fmt.Errorf("read member %q: %w", name.String(), err) + return false, fmt.Errorf("read member %q: %w", nameString, err) } if err := f.encoder.WriteValue(value); err != nil { - return false, fmt.Errorf("write member %q: %w", name.String(), err) + return false, fmt.Errorf("write member %q: %w", nameString, err) } } return false, nil } -// finish copies the members after the selected value and closes the object. +// finish closes a frame after its selected member has been resolved. +// The decoder must be positioned after the member's original value; +// the encoder must already contain the chosen replacement or omission. func (f *objectRewriteFrame) finish() (jsontext.Value, error) { for f.decoder.PeekKind() != '}' { name, err := f.decoder.ReadToken() @@ -442,13 +484,14 @@ func (f *objectRewriteFrame) finish() (jsontext.Value, error) { if err := f.encoder.WriteToken(name); err != nil { return nil, fmt.Errorf("write member name: %w", err) } + nameString := name.String() value, err := f.decoder.ReadValue() if err != nil { - return nil, fmt.Errorf("read member %q: %w", name.String(), err) + return nil, fmt.Errorf("read member %q: %w", nameString, err) } if err := f.encoder.WriteValue(value); err != nil { - return nil, fmt.Errorf("write member %q: %w", name.String(), err) + return nil, fmt.Errorf("write member %q: %w", nameString, err) } } @@ -492,6 +535,8 @@ func InsertAutoIncrement( must.Bef(value.IsValid(), "invalid JSON mutation value") value = value.Clone() + // Derive the member path inside Then so every replay uses the counter value + // read from that replay's document. return Increment(counter).Then(func(id int64) Program[int64] { member := object.AppendToken(strconv.FormatInt(id, 10)) return Block( @@ -554,6 +599,7 @@ func lookupObjectMember( if _, err := decoder.ReadToken(); err != nil { return nil, false, err } + // Scan only this object level and avoid decoding unrelated values. for decoder.PeekKind() != '}' { name, err := decoder.ReadToken() if err != nil { @@ -574,6 +620,8 @@ func lookupObjectMember( if !ok { return value, true, nil } + // Lookup does not rebuild ancestors, + // so it can recurse into the selected value and return directly. return lookupObjectMember(value, childMember, next) } return nil, false, nil diff --git a/internal/jsonmut/jsonmut_property_test.go b/internal/jsonmut/jsonmut_property_test.go index 0d70f057..74c8c262 100644 --- a/internal/jsonmut/jsonmut_property_test.go +++ b/internal/jsonmut/jsonmut_property_test.go @@ -69,12 +69,16 @@ func testMutationMatchesReference(t *rapid.T) { path := drawJSONPath(t, document) replacement := jsonValueGenerator(maxGeneratedJSONDepth). Draw(t, "replacement") - mode := rapid.SampledFrom([]referenceMutation{ + modes := []referenceMutation{ referenceSet, referenceSetIfAbsent, referenceInsert, referenceReplace, - }).Draw(t, "mode") + } + if len(path) > 0 { + modes = append(modes, referenceDelete) + } + mode := rapid.SampledFrom(modes).Draw(t, "mode") documentJSON := marshalJSON(t, document) original := documentJSON.Clone() @@ -119,6 +123,7 @@ const ( referenceSetIfAbsent referenceInsert referenceReplace + referenceDelete ) func (m referenceMutation) statement( @@ -134,6 +139,8 @@ func (m referenceMutation) statement( return jsonmut.Insert(path, value) case referenceReplace: return jsonmut.Replace(path, value) + case referenceDelete: + return jsonmut.Delete(path) default: panic("unknown reference mutation") } @@ -185,6 +192,8 @@ func referenceMutate( return nil, referenceAlreadyExists case referenceSetIfAbsent: return document, referenceNoError + case referenceDelete: + panic("delete path must not be the document root") default: return replacement, referenceNoError } @@ -204,6 +213,11 @@ func referenceMutate( return nil, referenceDoesNotExist case mode == referenceSetIfAbsent && exists: return document, referenceNoError + case mode == referenceDelete && !exists: + return nil, referenceDoesNotExist + case mode == referenceDelete: + delete(object, member) + return document, referenceNoError default: object[member] = replacement return document, referenceNoError diff --git a/internal/jsonmut/jsonmut_test.go b/internal/jsonmut/jsonmut_test.go index c8e672be..f3f35e12 100644 --- a/internal/jsonmut/jsonmut_test.go +++ b/internal/jsonmut/jsonmut_test.go @@ -348,6 +348,34 @@ func TestReplace_Missing(t *testing.T) { assert.ErrorIs(t, err, jsonmut.ErrNotExist) } +func TestDelete(t *testing.T) { + t.Parallel() + + updated, _, err := jsonmut.Apply( + jsontext.Value(`{ + "drafts": { + "7": {"body": "remove"}, + "8": {"body": "keep"} + } + }`), + jsonmut.Delete("/drafts/7"), + ) + require.NoError(t, err) + assert.JSONEq(t, `{ + "drafts": {"8": {"body": "keep"}} + }`, updated.String()) +} + +func TestDelete_missing(t *testing.T) { + t.Parallel() + + _, _, err := jsonmut.Apply( + jsontext.Value(`{"drafts": {}}`), + jsonmut.Delete("/drafts/7"), + ) + assert.ErrorIs(t, err, jsonmut.ErrNotExist) +} + func TestSet_missingParent(t *testing.T) { t.Parallel() diff --git a/internal/spice/state/review_draft.go b/internal/spice/state/review_draft.go index 753581da..baf26110 100644 --- a/internal/spice/state/review_draft.go +++ b/internal/spice/state/review_draft.go @@ -97,6 +97,51 @@ func (s *Store) UpdateReviewDraftBody( return nil } +// RemovePublishedReviewDrafts removes unchanged drafts after publication. +func (s *Store) RemovePublishedReviewDrafts( + ctx context.Context, + branch string, + published []review.Draft, +) error { + statements := make([]jsonmut.Statement, 0, len(published)) + for _, draft := range published { + stored := storeReviewDraft(draft) + path := jsontext.Pointer("/drafts").AppendToken(draft.ID.String()) + statements = append(statements, + jsonmut.Decode[*storedReviewDraft](path).Then( + func(current *storedReviewDraft) jsonmut.Statement { + if current == nil || *current != stored { + return jsonmut.Block() + } + return jsonmut.Delete(path) + }, + ), + ) + } + + // Forge submission happens before this mutation starts. + // Replaying the program removes only values that still match the request, + // leaving drafts added or edited while submission was in flight intact. + err := storage.UpdateJSON( + ctx, + s.db, + storage.JSONMutationRequest{ + Key: reviewDraftsJSON(branch), + IfMissing: jsontext.Value(`{}`), + Requires: []string{branchKey(branch)}, + Message: fmt.Sprintf("%v: remove published review drafts", branch), + }, + jsonmut.Block(statements...), + ) + if errors.Is(err, storage.ErrNotExist) { + return nil + } + if err != nil { + return fmt.Errorf("remove published review drafts: %w", err) + } + return nil +} + // LoadReviewDrafts retrieves the unpublished review comments for branch. // It returns nil when the branch has no review drafts. func (s *Store) LoadReviewDrafts( @@ -134,18 +179,6 @@ func (s *Store) LoadReviewDrafts( return drafts, nil } -// ClearReviewDrafts removes review draft state for branch. -func (s *Store) ClearReviewDrafts(ctx context.Context, branch string) error { - if err := s.db.Delete( - ctx, - reviewDraftsJSON(branch), - fmt.Sprintf("%v: clear review drafts", branch), - ); err != nil { - return fmt.Errorf("delete review drafts: %w", err) - } - return nil -} - func (s *Store) loadReviewDraftState( ctx context.Context, branch string, diff --git a/internal/spice/state/review_draft_publish_test.go b/internal/spice/state/review_draft_publish_test.go new file mode 100644 index 00000000..089b2cfd --- /dev/null +++ b/internal/spice/state/review_draft_publish_test.go @@ -0,0 +1,232 @@ +package state_test + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.abhg.dev/gs/internal/review" + "go.abhg.dev/gs/internal/spice/state" + "go.abhg.dev/gs/internal/spice/state/storage" +) + +func TestReviewDraftsPublishPreservesAddedDraft(t *testing.T) { + ctx := t.Context() + db := storage.NewDB(make(storage.MapBackend)) + store, err := state.InitStore(ctx, state.InitStoreRequest{ + DB: db, + Trunk: "main", + }) + require.NoError(t, err) + tx := store.BeginBranchTx() + require.NoError(t, tx.Upsert(ctx, state.UpsertRequest{ + Name: "feat", + Base: "main", + })) + require.NoError(t, tx.Commit(ctx, "track feat")) + + _, err = store.AddReviewDraft( + ctx, + "feat", + review.Draft{ + ID: 0, + Body: "First", + Anchor: review.Anchor{ + Path: "first.go", + StartLine: 1, + EndLine: 1, + }, + }, + ) + require.NoError(t, err) + published, err := store.LoadReviewDrafts(ctx, "feat") + require.NoError(t, err) + + added, err := store.AddReviewDraft( + ctx, + "feat", + review.Draft{ + ID: 0, + Body: "Second", + Anchor: review.Anchor{ + Path: "second.go", + StartLine: 2, + EndLine: 2, + }, + }, + ) + require.NoError(t, err) + require.NoError(t, store.RemovePublishedReviewDrafts( + ctx, "feat", published, + )) + + drafts, err := store.LoadReviewDrafts(ctx, "feat") + require.NoError(t, err) + require.NotNil(t, drafts) + assert.Equal(t, []review.Draft{added}, drafts) +} + +func TestReviewDraftsPublishPreservesEditedDraft(t *testing.T) { + ctx := t.Context() + db := storage.NewDB(make(storage.MapBackend)) + store, err := state.InitStore(ctx, state.InitStoreRequest{ + DB: db, + Trunk: "main", + }) + require.NoError(t, err) + tx := store.BeginBranchTx() + require.NoError(t, tx.Upsert(ctx, state.UpsertRequest{ + Name: "feat", + Base: "main", + })) + require.NoError(t, tx.Commit(ctx, "track feat")) + + added, err := store.AddReviewDraft( + ctx, + "feat", + review.Draft{ + ID: 0, + Body: "First", + Anchor: review.Anchor{ + Path: "first.go", + StartLine: 1, + EndLine: 1, + }, + }, + ) + require.NoError(t, err) + published, err := store.LoadReviewDrafts(ctx, "feat") + require.NoError(t, err) + + require.NoError(t, store.UpdateReviewDraftBody( + ctx, "feat", added.ID, "First edited", + )) + require.NoError(t, store.RemovePublishedReviewDrafts( + ctx, "feat", published, + )) + + drafts, err := store.LoadReviewDrafts(ctx, "feat") + require.NoError(t, err) + require.NotNil(t, drafts) + edited := added + edited.Body = "First edited" + assert.Equal(t, []review.Draft{edited}, drafts) +} + +func TestReviewDraftsPublishAfterBranchDeletion(t *testing.T) { + ctx := t.Context() + db := storage.NewDB(make(storage.MapBackend)) + store, err := state.InitStore(ctx, state.InitStoreRequest{ + DB: db, + Trunk: "main", + }) + require.NoError(t, err) + tx := store.BeginBranchTx() + require.NoError(t, tx.Upsert(ctx, state.UpsertRequest{ + Name: "feat", + Base: "main", + })) + require.NoError(t, tx.Commit(ctx, "track feat")) + + _, err = store.AddReviewDraft( + ctx, + "feat", + review.Draft{ + ID: 0, + Body: "First", + Anchor: review.Anchor{ + Path: "first.go", + StartLine: 1, + EndLine: 1, + }, + }, + ) + require.NoError(t, err) + published, err := store.LoadReviewDrafts(ctx, "feat") + require.NoError(t, err) + + tx = store.BeginBranchTx() + require.NoError(t, tx.Delete(ctx, "feat")) + require.NoError(t, tx.Commit(ctx, "untrack feat")) + require.NoError(t, store.RemovePublishedReviewDrafts( + ctx, + "feat", + published, + )) + + drafts, err := store.LoadReviewDrafts(ctx, "feat") + require.NoError(t, err) + assert.Nil(t, drafts) +} + +func TestReviewDraftsPublishPreservesNextID(t *testing.T) { + ctx := t.Context() + db := storage.NewDB(make(storage.MapBackend)) + store, err := state.InitStore(ctx, state.InitStoreRequest{ + DB: db, + Trunk: "main", + }) + require.NoError(t, err) + tx := store.BeginBranchTx() + require.NoError(t, tx.Upsert(ctx, state.UpsertRequest{ + Name: "feat", + Base: "main", + })) + require.NoError(t, tx.Commit(ctx, "track feat")) + + _, err = store.AddReviewDraft( + ctx, + "feat", + review.Draft{ + ID: 0, + Body: "First", + Anchor: review.Anchor{ + Path: "first.go", + StartLine: 1, + EndLine: 1, + }, + }, + ) + require.NoError(t, err) + _, err = store.AddReviewDraft( + ctx, + "feat", + review.Draft{ + ID: 0, + Body: "Second", + Anchor: review.Anchor{ + Path: "second.go", + StartLine: 2, + EndLine: 2, + }, + }, + ) + require.NoError(t, err) + published, err := store.LoadReviewDrafts(ctx, "feat") + require.NoError(t, err) + + require.NoError(t, store.RemovePublishedReviewDrafts( + ctx, "feat", published, + )) + + drafts, err := store.LoadReviewDrafts(ctx, "feat") + require.NoError(t, err) + require.NotNil(t, drafts) + assert.Empty(t, drafts) + + next, err := store.AddReviewDraft( + ctx, + "feat", + review.Draft{ + ID: 0, + Body: "Third", + Anchor: review.Anchor{ + Path: "first.go", + StartLine: 1, + EndLine: 1, + }, + }, + ) + require.NoError(t, err) + assert.Equal(t, review.DraftID(3), next.ID) +} diff --git a/internal/spice/state/review_draft_test.go b/internal/spice/state/review_draft_test.go index 5cfb3711..30f46787 100644 --- a/internal/spice/state/review_draft_test.go +++ b/internal/spice/state/review_draft_test.go @@ -63,10 +63,30 @@ func TestReviewDrafts(t *testing.T) { assert.Equal(t, "updated body", drafts[0].Body) assert.Equal(t, reply, drafts[1]) - require.NoError(t, store.ClearReviewDrafts(ctx, "feature")) + require.NoError(t, store.RemovePublishedReviewDrafts( + ctx, + "feature", + drafts, + )) drafts, err = store.LoadReviewDrafts(ctx, "feature") require.NoError(t, err) - assert.Nil(t, drafts) + assert.Empty(t, drafts) + + next, err := store.AddReviewDraft( + ctx, + "feature", + review.Draft{ + ID: 0, + Body: "next body", + Anchor: review.Anchor{ + Path: "main.go", + StartLine: 42, + EndLine: 42, + }, + }, + ) + require.NoError(t, err) + assert.Equal(t, review.DraftID(3), next.ID) } func TestReviewDraftsFollowBranchLifecycle(t *testing.T) { diff --git a/testdata/script/review_publish.txt b/testdata/script/review_publish.txt index 18e0c999..775de2b7 100644 --- a/testdata/script/review_publish.txt +++ b/testdata/script/review_publish.txt @@ -49,6 +49,10 @@ stderr '--approve and --request-changes' gs review list --draft-only stderr 'No draft comments' +# Published draft IDs are not reused. +gs review comment feature.go:3 -m 'One more thought.' +stderr 'Drafted comment 3 on feature.go:3' + -- repo/feature.go -- package main