From 8f56df2809aca8c40a5105245e1f0bb5b7bfdaf7 Mon Sep 17 00:00:00 2001 From: omartelo Date: Sat, 24 Oct 2026 07:33:38 -0300 Subject: [PATCH] feat(native): send a pull request's review threads to its session An open review thread on the Pulls screen gets Send to session beside Reply and Resolve, in the Conversation tab and in the diff, and the Conversation tab can send every open thread at once. The threads go as one prompt to the session in the branch's checkout, opened when there is none. Once the session took the prompt, lich answers each thread on GitHub and keeps it waiting (thread_fixes, schema version 12) for the pull request's next push. A watcher reads each waiting pull request's head once a minute and, when it moved, resolves the thread and replies with the new head's short sha. A pull request that closed, or a checkout that is gone, drops its threads. TestOpenDropsTheTerminalThemeSetting drops the new table with the other post-version-0 additions: the contract changed (a new migration), the assertion did not. --- CHANGELOG.md | 2 + api/testdata/contract.golden | 18 ++ api/threadfix.go | 32 +++ docs/ceilings.md | 8 + internal/project/project.go | 3 + internal/project/threadfix.go | 196 +++++++++++++++++++ internal/project/threadfix_test.go | 187 ++++++++++++++++++ internal/routes/project.go | 2 + internal/store/migrate.go | 17 ++ internal/store/store_test.go | 2 +- internal/store/threadfixes.go | 50 +++++ internal/store/threadfixes_test.go | 61 ++++++ main.go | 4 + native/backend/threadfix.go | 21 ++ native/cmd/lich-native/main.go | 5 + native/cmd/lich-native/model.go | 6 + native/cmd/lich-native/pulls.go | 4 + native/cmd/lich-native/pulls_actions.go | 5 + native/cmd/lich-native/pulls_conversation.go | 63 +++++- native/cmd/lich-native/pulls_threads.go | 111 +++++++++++ native/cmd/lich-native/pulls_threads_test.go | 68 +++++++ native/cmd/lich-native/pulls_view.go | 1 + native/cmd/lich-native/threadfix_store.go | 42 ++++ native/i18n/locales/en/prompts.json | 8 + native/i18n/locales/en/pulls.json | 14 ++ native/i18n/locales/es/prompts.json | 8 + native/i18n/locales/es/pulls.json | 14 ++ native/i18n/locales/ja/prompts.json | 8 + native/i18n/locales/ja/pulls.json | 14 ++ native/i18n/locales/ko/prompts.json | 8 + native/i18n/locales/ko/pulls.json | 14 ++ native/i18n/locales/pt-br/prompts.json | 8 + native/i18n/locales/pt-br/pulls.json | 14 ++ native/i18n/locales/ru/prompts.json | 10 + native/i18n/locales/ru/pulls.json | 18 ++ native/i18n/locales/zh-cn/prompts.json | 8 + native/i18n/locales/zh-cn/pulls.json | 14 ++ 37 files changed, 1061 insertions(+), 7 deletions(-) create mode 100644 api/threadfix.go create mode 100644 internal/project/threadfix.go create mode 100644 internal/project/threadfix_test.go create mode 100644 internal/store/threadfixes.go create mode 100644 internal/store/threadfixes_test.go create mode 100644 native/backend/threadfix.go create mode 100644 native/cmd/lich-native/pulls_threads.go create mode 100644 native/cmd/lich-native/pulls_threads_test.go create mode 100644 native/cmd/lich-native/threadfix_store.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 2cd39f37b..bfc76406c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - On macOS, lich has a menu-bar item: a lich whose window is closed shows it is still running its sessions there, with Show lich, the count of running sessions and Quit, as the tray does on Linux and Windows. - **Services: commands a project keeps running, started by lich.** Save a container, a tunnel or a dev server once in Settings › Services of a project, and start it from the Services row at the bottom of the sidebar, above the profile. Closed, the row says how many run (2/3); opened, it lists each service with how long it has run or how it ended, and Start, Stop and Restart on hover. A click opens its console, where it takes input and Ctrl+C stops it. A service is not a terminal: when its command exits, it has stopped, its last output stays readable, and Start runs it again. Tick Start with lich and it starts every time lich does with its project open. Services saved in Settings › Services with Global selected run in your home directory and show on lich's home screen, the place for a `lazydocker` you keep open whatever the project. A service lives as long as its project's tab: closing the tab stops it, and reopening the project starts the ones saved to start with lich. Closing the window leaves them running, and the tray lists them with Show output, Restart and Stop; quitting lich stops them, and the close dialog counts them. With the sidebar collapsed, the rail's Services button opens the same list. +- **Send a pull request's review threads to its session.** On the Pulls screen, an open review thread has Send to session beside Reply and Resolve, in the Conversation tab and in the diff, and the Conversation tab can send every open thread at once. The threads go as one prompt to the session in the branch's checkout, which lich opens when there is none, with each thread's file, lines, link and every comment in it. lich then answers each thread on GitHub with "Fixing this. It'll be in the next commit." and resolves it once the pull request is pushed again, replying with the commit it landed in; until then the thread reads Sent, waiting for a push. Any push resolves it, even one that did not touch it. + ### Changed - The quick filters over the pull request list in Pulls (All, Ready, Drafts, Failing) are lighter: small text with the selected one shaded, as in the dock's tabs, sharing the list's width. A filter with nothing in it is faded and Failing's count turns red when a check fails. They no longer run past the list's edge with a monospace interface font. diff --git a/api/testdata/contract.golden b/api/testdata/contract.golden index aacc4a61c..3e22d17a0 100644 --- a/api/testdata/contract.golden +++ b/api/testdata/contract.golden @@ -44,6 +44,7 @@ opencodeprofile.LoginCommand(ProfileSlugOptions) string opencodeprofile.Profiles(None) []ProfileInfo opencodeprofile.UpdateProfile(UpdateProfileOptions) Profile patchnotes.Current(PatchNotesOptions) PatchNotes +project.AwaitThreadFix(ThreadFixOptions) None project.BaseFreshness(PathBranchOptions) BaseFreshness project.BaseStatus(PathOptions) *BaseStatus project.BranchRules(PathBranchOptions) *BranchRules @@ -93,6 +94,7 @@ project.RestoreLines(RestoreLinesOptions) None project.RevertLines(RevertLinesOptions) RevertResult project.Search(SearchOptions) SearchResult project.SubmitReview(SubmitReviewOptions) None +project.ThreadFixes(None) []ThreadFix project.Tree(PathOptions) FileListing project.WorktreeAdopted(PathOptions) bool project.WorktreeDirty(PathOptions) bool @@ -247,6 +249,7 @@ event sessions-filed SessionsFiled event terminal:data: string event terminal:exit: SessionExit event theme-selection ThemeSelection +event thread-fixes []ThreadFix event window-close None event window-raise None @@ -356,6 +359,13 @@ PatchNotes { groups []PatchNotesGroup } +ThreadFixOptions { + path string + number int + threadId string + commentId int64 +} + PathBranchOptions { path string branch string @@ -636,6 +646,14 @@ SubmitReviewOptions { comments []ReviewComment } +ThreadFix { + threadId string + path string + number int + commentId int64 + head string +} + FileListing { files []string cut bool diff --git a/api/threadfix.go b/api/threadfix.go new file mode 100644 index 000000000..87a3951f8 --- /dev/null +++ b/api/threadfix.go @@ -0,0 +1,32 @@ +package api + +var ( + ProjectAwaitThreadFix = define[ThreadFixOptions, None]("project.AwaitThreadFix") + ProjectThreadFixes = define[None, []ThreadFix]("project.ThreadFixes") + + // ThreadFixesEvent carries every review thread waiting for its fix, emitted + // once one starts waiting or stops: resolved by a push, or dropped with + // its pull request. + ThreadFixesEvent = defineEvent[[]ThreadFix]("thread-fixes") +) + +// ThreadFixOptions names a review thread just handed to a session: ThreadID is +// its GraphQL node id (what resolving takes), CommentID the database id of a +// comment in it (what a reply takes). +type ThreadFixOptions struct { + Path string `json:"path"` + Number int `json:"number"` + ThreadID string `json:"threadId"` + CommentID int64 `json:"commentId"` +} + +// ThreadFix is a review thread waiting for the pull request's next push, which +// resolves it. Head is the commit the pull request stood on when the thread +// was sent. +type ThreadFix struct { + ThreadID string `json:"threadId"` + Path string `json:"path"` + Number int `json:"number"` + CommentID int64 `json:"commentId"` + Head string `json:"head"` +} diff --git a/docs/ceilings.md b/docs/ceilings.md index 71a9f5e5a..51b22ecba 100644 --- a/docs/ceilings.md +++ b/docs/ceilings.md @@ -490,6 +490,14 @@ work when nobody knows it and that the call site never shows. The mechanism and not hold it, under ReadFile's size cap. A relative image shows its alt text (the renderer fetches nothing) and raw HTML is left out. A new version of the file puts the card back on its diff, as it reopens or folds a card by its size. +- **Any push resolves a thread sent to a session** (`internal/project/threadfix.go`): Send to session on the + Pulls screen replies on the thread at once and resolves it when the pull request's head first moves after the + send, replying with that head's short sha. lich cannot tell the commit that fixed the thread from one that did + not, so an unrelated push resolves it too, and a fix that never gets pushed leaves it waiting. The head is read + once a minute per waiting pull request, so the resolve lands up to a minute after the push. A pull request + that closes, or a checkout that is gone, drops its threads without a word on GitHub. A thread whose resolve + GitHub keeps refusing (deleted since) is retried every minute until its pull request closes. The replies are + written in English, whatever the interface and prompt languages: the reviewer reading them is someone else. ## Persistence and history diff --git a/internal/project/project.go b/internal/project/project.go index b1a0e618b..75e9337a9 100644 --- a/internal/project/project.go +++ b/internal/project/project.go @@ -55,6 +55,9 @@ type Service struct { baseRefs refetchGuard // emit reports a pull request write (prchanged.go); nil reports nothing. emit func(name string, data any) + // fixes keeps the review threads waiting for a push (threadfix.go); nil + // keeps nothing. + fixes threadFixStore } // New returns a project service using the given picker. diff --git a/internal/project/threadfix.go b/internal/project/threadfix.go new file mode 100644 index 000000000..e67d1ef3c --- /dev/null +++ b/internal/project/threadfix.go @@ -0,0 +1,196 @@ +package project + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "io/fs" + "log/slog" + "os" + "time" + + "github.com/lichdotdev/lich/api" + "github.com/lichdotdev/lich/internal/events" +) + +// A review thread handed to a session (the Pulls screen's Send to session) is +// answered on GitHub at once and resolved once the pull request moves: the +// first head after the send is taken as the fix. Resolving at the send would +// mark a thread done before any code changed; waiting for the push trades that +// for the opposite risk, an unrelated push resolving it (docs/ceilings.md). +// +// The replies are English on purpose: the reviewer reading them is someone +// else, whose language lich has no setting for. +const ( + threadFixingReply = "Fixing this. It'll be in the next commit." + threadFixedReply = "Fixed in %s." + // threadFixPoll is how often the waiting pull requests are read, one gh + // call each, and only while something waits. + threadFixPoll = time.Minute + // shortSHA is the length git itself abbreviates a commit to, which GitHub + // links in a comment. + shortSHA = 7 + prOpen = "OPEN" +) + +// threadFixStore keeps the waiting threads across restarts; main wires the +// workspace store, and nil keeps nothing. +type threadFixStore interface { + ThreadFixes() ([]api.ThreadFix, error) + SaveThreadFix(api.ThreadFix) error + DeleteThreadFix(threadID string) error +} + +// SetThreadFixes wires where the waiting threads are kept. +func (s *Service) SetThreadFixes(store threadFixStore) { s.fixes = store } + +type prTip struct { + HeadRefOID string `json:"headRefOid"` + State string `json:"state"` +} + +type prKey struct { + path string + number int +} + +func (s *Service) readTip(path string, number int) (prTip, error) { + out, err := s.gh(prReadTimeout, path, prArgs("view", number, "--json", "headRefOid,state")...) + if err != nil { + return prTip{}, err + } + var head prTip + if err := json.Unmarshal(out, &head); err != nil { + return prTip{}, fmt.Errorf("could not read the pull request's head: %w", err) + } + return head, nil +} + +// AwaitThreadFix answers a thread just handed to a session and has it wait for +// the pull request's next push. The head is read here rather than taken from +// the window, whose read may predate a push that would resolve the thread at +// once. +func (s *Service) AwaitThreadFix(ctx context.Context, opts api.ThreadFixOptions) (api.None, error) { + if opts.ThreadID == "" || opts.Number <= 0 || opts.CommentID == 0 { + return api.None{}, errors.New("a pull request, a thread and one of its comments are required") + } + if s.fixes == nil { + return api.None{}, errors.New("nowhere to keep the thread while it waits") + } + head, err := s.readTip(opts.Path, opts.Number) + if err != nil { + return api.None{}, err + } + if head.State != prOpen { + return api.None{}, errors.New("the pull request is not open") + } + reply := api.PRCommentBodyOptions{Path: opts.Path, Number: opts.Number, CommentID: opts.CommentID, Body: threadFixingReply} + if _, err := s.ReplyToReviewThread(ctx, reply); err != nil { + return api.None{}, err + } + fix := api.ThreadFix{ThreadID: opts.ThreadID, Path: opts.Path, Number: opts.Number, CommentID: opts.CommentID, Head: head.HeadRefOID} + if err := s.fixes.SaveThreadFix(fix); err != nil { + return api.None{}, err + } + s.emitThreadFixes() + return api.None{}, nil +} + +// ThreadFixes is every thread waiting for its fix. +func (s *Service) ThreadFixes(_ context.Context, _ api.None) ([]api.ThreadFix, error) { + if s.fixes == nil { + return nil, nil + } + return s.fixes.ThreadFixes() +} + +// RunThreadFixWatch settles the waiting threads every threadFixPoll, for as +// long as lich runs. +func (s *Service) RunThreadFixWatch() { + ticker := time.NewTicker(threadFixPoll) + defer ticker.Stop() + for range ticker.C { + s.checkThreadFixes() + } +} + +// checkThreadFixes reads each waiting pull request once and settles its +// threads. A read or a write that fails leaves the thread waiting for the next +// pass. +func (s *Service) checkThreadFixes() { + fixes, err := s.ThreadFixes(context.Background(), api.None{}) + if err != nil { + slog.Warn("thread fixes: list", "err", err) + return + } + heads := map[prKey]prTip{} + unread := map[prKey]bool{} + changed := false + for _, f := range fixes { + key := prKey{f.Path, f.Number} + if unread[key] { + continue + } + head, ok := heads[key] + if !ok { + if head, err = s.waitingHead(f); err != nil { + slog.Warn("thread fixes: read the pull request", "path", f.Path, "number", f.Number, "err", err) + unread[key] = true + continue + } + heads[key] = head + } + settled, err := s.settleThreadFix(f, head) + if err != nil { + slog.Warn("thread fixes: settle", "thread", f.ThreadID, "err", err) + } + changed = changed || settled + } + if changed { + s.emitThreadFixes() + } +} + +// waitingHead reads f's pull request, as closed when its checkout is gone: +// gh has nothing left to run in, and nothing will ever push from it. +func (s *Service) waitingHead(f api.ThreadFix) (prTip, error) { + if _, err := os.Stat(f.Path); errors.Is(err, fs.ErrNotExist) { + return prTip{}, nil + } + return s.readTip(f.Path, f.Number) +} + +// settleThreadFix resolves f once its pull request moved past the head it was +// sent at, and drops it once the pull request closed; it reports whether f +// stopped waiting. Resolving goes first: a reply that fails after it is +// retried with a resolve that changes nothing, never a second reply. +func (s *Service) settleThreadFix(f api.ThreadFix, head prTip) (bool, error) { + if head.State != prOpen { + return true, s.fixes.DeleteThreadFix(f.ThreadID) + } + if head.HeadRefOID == f.Head { + return false, nil + } + if _, err := s.ResolveReviewThread(context.Background(), api.ResolveThreadOptions{Path: f.Path, ThreadID: f.ThreadID, Resolved: true}); err != nil { + return false, err + } + sha := head.HeadRefOID[:min(shortSHA, len(head.HeadRefOID))] + reply := api.PRCommentBodyOptions{Path: f.Path, Number: f.Number, CommentID: f.CommentID, Body: fmt.Sprintf(threadFixedReply, sha)} + if _, err := s.ReplyToReviewThread(context.Background(), reply); err != nil { + return false, err + } + return true, s.fixes.DeleteThreadFix(f.ThreadID) +} + +func (s *Service) emitThreadFixes() { + if s.emit == nil { + return + } + fixes, err := s.ThreadFixes(context.Background(), api.None{}) + if err != nil { + slog.Warn("thread fixes: list", "err", err) + return + } + events.Send(s.emit, api.ThreadFixesEvent, fixes) +} diff --git a/internal/project/threadfix_test.go b/internal/project/threadfix_test.go new file mode 100644 index 000000000..649e7ea4d --- /dev/null +++ b/internal/project/threadfix_test.go @@ -0,0 +1,187 @@ +package project + +import ( + "errors" + "slices" + "strings" + "testing" + + "github.com/lichdotdev/lich/api" +) + +// memFixes stands in for the workspace store, the database boundary. +type memFixes struct{ list []api.ThreadFix } + +func (m *memFixes) ThreadFixes() ([]api.ThreadFix, error) { return slices.Clone(m.list), nil } + +func (m *memFixes) SaveThreadFix(f api.ThreadFix) error { + m.list = slices.DeleteFunc(m.list, func(o api.ThreadFix) bool { return o.ThreadID == f.ThreadID }) + m.list = append(m.list, f) + return nil +} + +func (m *memFixes) DeleteThreadFix(id string) error { + m.list = slices.DeleteFunc(m.list, func(o api.ThreadFix) bool { return o.ThreadID == id }) + return nil +} + +type emitted struct{ names []string } + +func (e *emitted) emit(name string, _ any) { e.names = append(e.names, name) } + +func withFixes(gh *fakeGH, fixes *memFixes, ev *emitted) *Service { + s := withGH(gh) + s.SetThreadFixes(fixes) + s.SetEmit(ev.emit) + return s +} + +func TestAwaitThreadFixRepliesAndWaitsOnTheHeadItReads(t *testing.T) { + t.Parallel() + gh := &fakeGH{outs: [][]byte{[]byte(`{"headRefOid":"abc1234567","state":"OPEN"}`)}} + fixes, ev := &memFixes{}, &emitted{} + opts := api.ThreadFixOptions{Path: "/repo", Number: 42, ThreadID: "PRRT_1", CommentID: 900001} + if _, err := withFixes(gh, fixes, ev).AwaitThreadFix(t.Context(), opts); err != nil { + t.Fatalf("AwaitThreadFix: %v", err) + } + if want := []string{"pr", "view", "42", "--json", "headRefOid,state"}; !slices.Equal(gh.history[0], want) { + t.Errorf("read = %v, want %v", gh.history[0], want) + } + if reply := gh.history[1]; !slices.Contains(reply, "repos/{owner}/{repo}/pulls/42/comments/900001/replies") || !slices.Contains(reply, "body="+threadFixingReply) { + t.Errorf("reply = %v, want the fixing reply on the thread's comment", reply) + } + want := []api.ThreadFix{{ThreadID: "PRRT_1", Path: "/repo", Number: 42, CommentID: 900001, Head: "abc1234567"}} + if !slices.Equal(fixes.list, want) { + t.Errorf("waiting = %+v, want %+v", fixes.list, want) + } + if !slices.Contains(ev.names, api.ThreadFixesEvent.Name) { + t.Errorf("events = %v, want %s", ev.names, api.ThreadFixesEvent.Name) + } +} + +func TestAwaitThreadFixRefusals(t *testing.T) { + t.Parallel() + open := api.ThreadFixOptions{Path: "/repo", Number: 42, ThreadID: "PRRT_1", CommentID: 1} + t.Run("a closed pull request never gets the reply", func(t *testing.T) { + gh := &fakeGH{out: []byte(`{"headRefOid":"abc","state":"MERGED"}`)} + fixes := &memFixes{} + if _, err := withFixes(gh, fixes, &emitted{}).AwaitThreadFix(t.Context(), open); err == nil { + t.Error("expected an error for a merged pull request") + } + if gh.calls != 1 || len(fixes.list) != 0 { + t.Errorf("gh calls = %d, waiting = %v; want only the read and nothing waiting", gh.calls, fixes.list) + } + }) + t.Run("a failed reply leaves nothing waiting", func(t *testing.T) { + gh := &fakeGH{outs: [][]byte{[]byte(`{"headRefOid":"abc","state":"OPEN"}`)}, errs: []error{nil, errors.New("HTTP 403")}} + fixes := &memFixes{} + if _, err := withFixes(gh, fixes, &emitted{}).AwaitThreadFix(t.Context(), open); err == nil { + t.Error("expected the reply's error") + } + if len(fixes.list) != 0 { + t.Errorf("waiting = %v, want nothing", fixes.list) + } + }) + t.Run("no thread never reaches gh", func(t *testing.T) { + gh := &fakeGH{} + bad := open + bad.ThreadID = "" + if _, err := withFixes(gh, &memFixes{}, &emitted{}).AwaitThreadFix(t.Context(), bad); err == nil || gh.calls != 0 { + t.Errorf("err = %v, gh calls = %d; want an error and no call", err, gh.calls) + } + }) +} + +func TestCheckThreadFixesResolvesOnANewHead(t *testing.T) { + t.Parallel() + dir := t.TempDir() + gh := &fakeGH{outs: [][]byte{[]byte(`{"headRefOid":"def4567890","state":"OPEN"}`)}} + fixes := &memFixes{list: []api.ThreadFix{ + {ThreadID: "PRRT_1", Path: dir, Number: 42, CommentID: 11, Head: "abc"}, + {ThreadID: "PRRT_2", Path: dir, Number: 42, CommentID: 12, Head: "abc"}, + }} + ev := &emitted{} + withFixes(gh, fixes, ev).checkThreadFixes() + reads := 0 + for _, call := range gh.history { + if slices.Contains(call, "view") { + reads++ + } + } + if reads != 1 { + t.Errorf("pull request reads = %d, want one for both threads", reads) + } + joined := strings.Join(slices.Concat(gh.history...), " ") + for _, want := range []string{"id=PRRT_1", "id=PRRT_2", "comments/11/replies", "comments/12/replies", "body=Fixed in def4567."} { + if !strings.Contains(joined, want) { + t.Errorf("gh calls lack %q: %v", want, gh.history) + } + } + if len(fixes.list) != 0 { + t.Errorf("waiting = %v, want nothing", fixes.list) + } + if !slices.Contains(ev.names, api.ThreadFixesEvent.Name) { + t.Errorf("events = %v, want %s", ev.names, api.ThreadFixesEvent.Name) + } +} + +func TestCheckThreadFixesKeepsWaiting(t *testing.T) { + t.Parallel() + dir := t.TempDir() + waiting := api.ThreadFix{ThreadID: "PRRT_1", Path: dir, Number: 42, CommentID: 11, Head: "abc"} + t.Run("on the same head", func(t *testing.T) { + gh := &fakeGH{out: []byte(`{"headRefOid":"abc","state":"OPEN"}`)} + fixes, ev := &memFixes{list: []api.ThreadFix{waiting}}, &emitted{} + withFixes(gh, fixes, ev).checkThreadFixes() + if gh.calls != 1 || len(fixes.list) != 1 || len(ev.names) != 0 { + t.Errorf("gh calls = %d, waiting = %v, events = %v; want one read, still waiting, no event", gh.calls, fixes.list, ev.names) + } + }) + t.Run("when the resolve fails, with no reply sent", func(t *testing.T) { + gh := &fakeGH{outs: [][]byte{[]byte(`{"headRefOid":"def","state":"OPEN"}`)}, errs: []error{nil, errors.New("HTTP 502")}} + fixes := &memFixes{list: []api.ThreadFix{waiting}} + withFixes(gh, fixes, &emitted{}).checkThreadFixes() + if gh.calls != 2 || len(fixes.list) != 1 { + t.Errorf("gh calls = %d, waiting = %v; want the read and the resolve, still waiting", gh.calls, fixes.list) + } + }) + t.Run("when the read fails", func(t *testing.T) { + gh := &fakeGH{err: errors.New("offline")} + fixes := &memFixes{list: []api.ThreadFix{waiting, {ThreadID: "PRRT_2", Path: dir, Number: 42, CommentID: 12, Head: "abc"}}} + withFixes(gh, fixes, &emitted{}).checkThreadFixes() + if gh.calls != 1 || len(fixes.list) != 2 { + t.Errorf("gh calls = %d, waiting = %v; want one read for the pull request, both still waiting", gh.calls, fixes.list) + } + }) +} + +func TestCheckThreadFixesDropsWhatCanNoLongerBeFixed(t *testing.T) { + t.Parallel() + t.Run("a merged pull request", func(t *testing.T) { + gh := &fakeGH{out: []byte(`{"headRefOid":"abc","state":"MERGED"}`)} + fixes := &memFixes{list: []api.ThreadFix{{ThreadID: "PRRT_1", Path: t.TempDir(), Number: 42, CommentID: 11, Head: "abc"}}} + withFixes(gh, fixes, &emitted{}).checkThreadFixes() + if gh.calls != 1 || len(fixes.list) != 0 { + t.Errorf("gh calls = %d, waiting = %v; want the read only and nothing waiting", gh.calls, fixes.list) + } + }) + t.Run("a checkout that is gone", func(t *testing.T) { + gh := &fakeGH{} + fixes := &memFixes{list: []api.ThreadFix{{ThreadID: "PRRT_1", Path: t.TempDir() + "/gone", Number: 42, CommentID: 11, Head: "abc"}}} + withFixes(gh, fixes, &emitted{}).checkThreadFixes() + if gh.calls != 0 || len(fixes.list) != 0 { + t.Errorf("gh calls = %d, waiting = %v; want no call and nothing waiting", gh.calls, fixes.list) + } + }) +} + +func TestThreadFixesUnwired(t *testing.T) { + t.Parallel() + s := withGH(&fakeGH{}) + if got, err := s.ThreadFixes(t.Context(), api.None{}); err != nil || got != nil { + t.Errorf("ThreadFixes() = %v, %v; want nothing", got, err) + } + if _, err := s.AwaitThreadFix(t.Context(), api.ThreadFixOptions{Path: "/repo", Number: 1, ThreadID: "PRRT_1", CommentID: 1}); err == nil { + t.Error("expected an error with nowhere to keep the thread") + } +} diff --git a/internal/routes/project.go b/internal/routes/project.go index 19be0cd6a..41b646e0e 100644 --- a/internal/routes/project.go +++ b/internal/routes/project.go @@ -80,4 +80,6 @@ func projectPullRequestWrites(d *rpc.Handler, s *project.Service) { rpc.Handle(d, api.ProjectDeleteReviewComment, s.DeleteReviewComment) rpc.Handle(d, api.ProjectReplyToReviewThread, s.ReplyToReviewThread) rpc.Handle(d, api.ProjectResolveReviewThread, s.ResolveReviewThread) + rpc.Handle(d, api.ProjectAwaitThreadFix, s.AwaitThreadFix) + rpc.Handle(d, api.ProjectThreadFixes, s.ThreadFixes) } diff --git a/internal/store/migrate.go b/internal/store/migrate.go index 5f6f9dc7e..553baa2a0 100644 --- a/internal/store/migrate.go +++ b/internal/store/migrate.go @@ -24,6 +24,7 @@ var migrations = []func(*sql.Tx) error{ addSessionProfile, addSessionForked, addProjectServices, + addThreadFixes, } // addSessionEffort is version 2: the reasoning effort a session was opened at, @@ -151,6 +152,22 @@ CREATE TABLE project_services ( return err } +// addThreadFixes is version 12: the review threads handed to a session and +// waiting for the pull request's next push, which resolves them +// (project.WatchThreadFixes). It outlives the window, because the push comes +// whenever the agent gets there. +func addThreadFixes(tx *sql.Tx) error { + _, err := tx.Exec(` +CREATE TABLE thread_fixes ( + thread_id TEXT NOT NULL PRIMARY KEY, + path TEXT NOT NULL, + number INTEGER NOT NULL, + comment_id INTEGER NOT NULL, + head TEXT NOT NULL +)`) + return err +} + // NewerSchemaError is an open refused because the database was written by a // newer lich than this one. Opening it anyway would let this binary write its // defaults over columns it does not know about, so it is not opened at all. diff --git a/internal/store/store_test.go b/internal/store/store_test.go index f2f893fa2..f8fee95bc 100644 --- a/internal/store/store_test.go +++ b/internal/store/store_test.go @@ -1026,7 +1026,7 @@ func TestOpenDropsTheTerminalThemeSetting(t *testing.T) { _, _ = before.SetSetting(t.Context(), api.SetSettingOptions{Key: "appearance.terminalTheme", ProjectID: "p1", Value: "emerald"}) _, _ = before.SetSetting(t.Context(), api.SetSettingOptions{Key: "appearance.theme", Value: "rose-pine"}) _ = before.Close() - seedDB(t, path, `ALTER TABLE sessions DROP COLUMN effort; ALTER TABLE sessions DROP COLUMN ultracode; ALTER TABLE sessions DROP COLUMN subagent; ALTER TABLE session_costs DROP COLUMN reported; ALTER TABLE sessions DROP COLUMN color; ALTER TABLE sessions DROP COLUMN profile; ALTER TABLE sessions DROP COLUMN forked; DROP TABLE provider_session_tombstones; DROP TRIGGER sessions_tombstone_ad; DROP TRIGGER sessions_tombstone_ai; DROP TRIGGER sessions_tombstone_au; DROP TABLE project_services; PRAGMA user_version = 0`) + seedDB(t, path, `ALTER TABLE sessions DROP COLUMN effort; ALTER TABLE sessions DROP COLUMN ultracode; ALTER TABLE sessions DROP COLUMN subagent; ALTER TABLE session_costs DROP COLUMN reported; ALTER TABLE sessions DROP COLUMN color; ALTER TABLE sessions DROP COLUMN profile; ALTER TABLE sessions DROP COLUMN forked; DROP TABLE provider_session_tombstones; DROP TRIGGER sessions_tombstone_ad; DROP TRIGGER sessions_tombstone_ai; DROP TRIGGER sessions_tombstone_au; DROP TABLE project_services; DROP TABLE thread_fixes; PRAGMA user_version = 0`) after, err := open(path) if err != nil { diff --git a/internal/store/threadfixes.go b/internal/store/threadfixes.go new file mode 100644 index 000000000..b4990ac7c --- /dev/null +++ b/internal/store/threadfixes.go @@ -0,0 +1,50 @@ +package store + +import ( + "fmt" + + "github.com/lichdotdev/lich/api" +) + +// ThreadFixes returns every review thread waiting for its fix, in the order +// they were sent. +func (s *Service) ThreadFixes() ([]api.ThreadFix, error) { + rows, err := s.db.Query(`SELECT thread_id, path, number, comment_id, head FROM thread_fixes ORDER BY rowid`) + if err != nil { + return nil, fmt.Errorf("query thread fixes: %w", err) + } + defer rows.Close() + var out []api.ThreadFix + for rows.Next() { + var f api.ThreadFix + if err := rows.Scan(&f.ThreadID, &f.Path, &f.Number, &f.CommentID, &f.Head); err != nil { + return nil, fmt.Errorf("scan thread fix: %w", err) + } + out = append(out, f) + } + return out, rows.Err() +} + +// SaveThreadFix records f, replacing what an earlier send of the same thread +// recorded: the thread waits on the head it was last sent at. +func (s *Service) SaveThreadFix(f api.ThreadFix) error { + _, err := s.db.Exec( + `INSERT INTO thread_fixes (thread_id, path, number, comment_id, head) VALUES (?, ?, ?, ?, ?) + ON CONFLICT (thread_id) DO UPDATE SET + path = excluded.path, number = excluded.number, comment_id = excluded.comment_id, head = excluded.head`, + f.ThreadID, f.Path, f.Number, f.CommentID, f.Head, + ) + if err != nil { + return fmt.Errorf("save thread fix: %w", err) + } + return nil +} + +// DeleteThreadFix stops thread id waiting. One that is not waiting is not an +// error. +func (s *Service) DeleteThreadFix(threadID string) error { + if _, err := s.db.Exec(`DELETE FROM thread_fixes WHERE thread_id = ?`, threadID); err != nil { + return fmt.Errorf("delete thread fix: %w", err) + } + return nil +} diff --git a/internal/store/threadfixes_test.go b/internal/store/threadfixes_test.go new file mode 100644 index 000000000..95505d0bb --- /dev/null +++ b/internal/store/threadfixes_test.go @@ -0,0 +1,61 @@ +package store + +import ( + "slices" + "testing" + + "github.com/lichdotdev/lich/api" +) + +func TestThreadFixesRoundTripInSendOrder(t *testing.T) { + svc := newTestStore(t) + want := []api.ThreadFix{ + {ThreadID: "PRRT_b", Path: "/repo", Number: 7, CommentID: 11, Head: "aaa"}, + {ThreadID: "PRRT_a", Path: "/repo", Number: 7, CommentID: 12, Head: "aaa"}, + } + for _, f := range want { + if err := svc.SaveThreadFix(f); err != nil { + t.Fatal(err) + } + } + got, err := svc.ThreadFixes() + if err != nil { + t.Fatal(err) + } + if !slices.Equal(got, want) { + t.Errorf("ThreadFixes() = %+v, want %+v", got, want) + } +} + +func TestSaveThreadFixAgainWaitsOnTheNewHead(t *testing.T) { + svc := newTestStore(t) + if err := svc.SaveThreadFix(api.ThreadFix{ThreadID: "PRRT_a", Path: "/repo", Number: 7, CommentID: 11, Head: "aaa"}); err != nil { + t.Fatal(err) + } + again := api.ThreadFix{ThreadID: "PRRT_a", Path: "/repo", Number: 7, CommentID: 13, Head: "bbb"} + if err := svc.SaveThreadFix(again); err != nil { + t.Fatal(err) + } + got, err := svc.ThreadFixes() + if err != nil { + t.Fatal(err) + } + if !slices.Equal(got, []api.ThreadFix{again}) { + t.Errorf("ThreadFixes() = %+v, want only %+v", got, again) + } +} + +func TestDeleteThreadFix(t *testing.T) { + svc := newTestStore(t) + if err := svc.SaveThreadFix(api.ThreadFix{ThreadID: "PRRT_a", Path: "/repo", Number: 7, CommentID: 11, Head: "aaa"}); err != nil { + t.Fatal(err) + } + for range 2 { + if err := svc.DeleteThreadFix("PRRT_a"); err != nil { + t.Fatalf("DeleteThreadFix: %v", err) + } + } + if got, err := svc.ThreadFixes(); err != nil || len(got) != 0 { + t.Errorf("ThreadFixes() = %+v, %v; want none", got, err) + } +} diff --git a/main.go b/main.go index 2dd645a26..1323e5e0b 100644 --- a/main.go +++ b/main.go @@ -222,6 +222,10 @@ func registerServices(db *store.Service, term *terminal.Service, svcs *services. // directory, so it validates the picked one against the workspace. proj.SetProjects(db.ProjectAt) proj.SetEmit(hub.Emit) + // A review thread handed to a session waits for the pull request's next + // push across restarts, since the push comes whenever the agent gets there. + proj.SetThreadFixes(db) + go proj.RunThreadFixWatch() // Parking a session records the branch its checkout was on, so the history // search can match a branch the row is showing (store.SetBranchOf), and a // copy of what was said in it, capped, so the same search reaches the diff --git a/native/backend/threadfix.go b/native/backend/threadfix.go new file mode 100644 index 000000000..e258fab0e --- /dev/null +++ b/native/backend/threadfix.go @@ -0,0 +1,21 @@ +package backend + +import ( + "context" + + "github.com/lichdotdev/lich/api" +) + +type ThreadFix = api.ThreadFix + +// AwaitThreadFix answers a thread just handed to a session on GitHub and has +// it resolve on the pull request's next push. +func (c *Client) AwaitThreadFix(ctx context.Context, opts api.ThreadFixOptions) error { + _, err := callMethod(ctx, c, api.ProjectAwaitThreadFix, opts) + return err +} + +// ThreadFixes is every review thread waiting for its push. +func (c *Client) ThreadFixes(ctx context.Context) ([]ThreadFix, error) { + return callMethod(ctx, c, api.ProjectThreadFixes, api.None{}) +} diff --git a/native/cmd/lich-native/main.go b/native/cmd/lich-native/main.go index 4a0d83488..b9f1e562a 100644 --- a/native/cmd/lich-native/main.go +++ b/native/cmd/lich-native/main.go @@ -229,6 +229,8 @@ func run(w *app.Window, rt backend.Runtime) error { ids, _, _ := u.model.sessionsAt(projectID, path) return ids } + u.pulls.sessionLabel = u.model.sessionLabel + u.pulls.waiting = u.model.threadFixes.has u.model.prChanged = u.prChanged u.model.themeSelection = u.appearance.applySelection u.model.exited = u.sessionExited @@ -372,6 +374,9 @@ func (u *window) connect() error { if err := u.model.services.load(u.ctx, u.client); err != nil { return err } + if err := u.model.threadFixes.load(u.ctx, u.client); err != nil { + return err + } // A stored value this build cannot read starts the window with no // layouts and says so; the next split stores over it. if err := u.model.loadLayouts(u.ctx); err != nil { diff --git a/native/cmd/lich-native/model.go b/native/cmd/lich-native/model.go index b8ff26f7d..bc1a2f110 100644 --- a/native/cmd/lich-native/model.go +++ b/native/cmd/lich-native/model.go @@ -108,6 +108,8 @@ type model struct { closed *closedProjects // services is every project service and its state. services servicesStore + // threadFixes is the review threads waiting for a push. + threadFixes threadFixStore // console is the service whose console is on screen in place of the // active session, "" for none. console string @@ -324,6 +326,10 @@ func (m *model) follow(ctx context.Context, events <-chan backend.Event) { if list, err := backend.Decode(ev, api.ServicesEvent); err == nil { m.services.set(list) } + case api.ThreadFixesEvent.Name: + if list, err := backend.Decode(ev, api.ThreadFixesEvent); err == nil { + m.threadFixes.set(list) + } case api.ServiceRevealEvent.Name: if r, err := backend.Decode(ev, api.ServiceRevealEvent); err == nil && m.serviceReveal != nil { m.serviceReveal(r.ID) diff --git a/native/cmd/lich-native/pulls.go b/native/cmd/lich-native/pulls.go index f298a1939..7ff89b7a7 100644 --- a/native/cmd/lich-native/pulls.go +++ b/native/cmd/lich-native/pulls.go @@ -103,6 +103,10 @@ type pullsScreen struct { files filesInput // sessionsIn is the project's sessions sitting in a checkout. sessionsIn func(projectID, path string) []string + // sessionLabel names a session. + sessionLabel func(id string) string + // waiting is whether a review thread waits for the push that resolves it. + waiting func(threadID string) bool } // reviewSession is where the pull request's comments go: a session in the diff --git a/native/cmd/lich-native/pulls_actions.go b/native/cmd/lich-native/pulls_actions.go index 7969d94fd..df057b8af 100644 --- a/native/cmd/lich-native/pulls_actions.go +++ b/native/cmd/lich-native/pulls_actions.go @@ -103,6 +103,11 @@ func (u *window) applyPulls(act pullsAction) { u.applyTalk(path, p.selected, act) u.applyEdit(path, p.selected, act) } + if len(act.threads) > 0 && p.selected != 0 { + if d := p.store.detail(path, p.selected).val; d != nil { + go u.sendThreads(p.project, *d, act.threads) + } + } if act.approve && p.selected != 0 { go u.approvePR(path, p.selected) } diff --git a/native/cmd/lich-native/pulls_conversation.go b/native/cmd/lich-native/pulls_conversation.go index 70a3366ec..e6769f05a 100644 --- a/native/cmd/lich-native/pulls_conversation.go +++ b/native/cmd/lich-native/pulls_conversation.go @@ -2,6 +2,7 @@ package main import ( "fmt" + "image" "strings" "time" @@ -25,6 +26,8 @@ type talkState struct { sendClicks map[string]*widget.Clickable cancelClicks map[string]*widget.Clickable resolveClick map[string]*widget.Clickable + toSession map[string]*widget.Clickable + allToSession widget.Clickable editing map[string]bool focusEdit string menus map[string]*ui.MenuState @@ -59,6 +62,7 @@ func (s *talkState) init() { s.replying, s.replyFields = map[string]bool{}, map[string]*replyField{} s.replyClicks, s.sendClicks = map[string]*widget.Clickable{}, map[string]*widget.Clickable{} s.cancelClicks, s.resolveClick = map[string]*widget.Clickable{}, map[string]*widget.Clickable{} + s.toSession = map[string]*widget.Clickable{} s.editing, s.menus, s.moreClicks = map[string]bool{}, map[string]*ui.MenuState{}, map[string]*widget.Clickable{} s.discards, s.edits = map[int]*widget.Clickable{}, map[int]*widget.Clickable{} } @@ -132,6 +136,9 @@ func (v *pullView) conversationRows(gtx C, th *ui.Theme, p *pullsScreen, d backe now := gtx.Now v.talkClicks(gtx, v.pendingOf(d.URL)) rows := v.pendingRows(th, v.pendingOf(d.URL)) + if open := p.sendableThreads(read.val); canSendThreads(d) && len(open) > 0 { + rows = append(rows, v.sendOpenThreads(th, p, d, open, act)) + } if len(items) == 0 && len(resolved) == 0 { rows = append(rows, func(gtx C) D { return panelNotice(gtx, th, i18n.T("pulls.pullsConversation.nothingYet")) }) } @@ -237,7 +244,7 @@ func (v *pullView) threadBox(gtx C, th *ui.Theme, p *pullsScreen, d backend.PRDe return layout.UniformInset(ui.Space(3)).Layout(gtx, func(gtx C) D { return layout.Flex{Axis: layout.Vertical}.Layout(gtx, layout.Rigid(func(gtx C) D { - return foldClick.Layout(gtx, func(gtx C) D { return threadHeader(gtx, on, t, fold.Value) }) + return foldClick.Layout(gtx, func(gtx C) D { return threadHeader(gtx, on, t, fold.Value, p.threadWaiting(t.ID)) }) }), layout.Rigid(func(gtx C) D { if !fold.Value { @@ -250,7 +257,7 @@ func (v *pullView) threadBox(gtx C, th *ui.Theme, p *pullsScreen, d backend.PRDe }) } -func threadHeader(gtx C, th *ui.Theme, t backend.PRReviewThread, open bool) D { +func threadHeader(gtx C, th *ui.Theme, t backend.PRReviewThread, open, waiting bool) D { chevron := icPullRight if open { chevron = icPullDown @@ -270,6 +277,9 @@ func threadHeader(gtx C, th *ui.Theme, t backend.PRReviewThread, open bool) D { if t.IsOutdated { children = append(children, ui.Gap(2), layout.Rigid(th.Text(ui.TextXS, i18n.T("pulls.pullsConversation.outdated")).In(th.ToneWait).Layout)) } + if waiting && !t.IsResolved { + children = append(children, ui.Gap(2), layout.Rigid(th.Text(ui.TextXS, i18n.T("pulls.threadSend.waiting")).Muted().Layout)) + } if t.IsResolved { children = append(children, ui.Gap(2), layout.Rigid(func(gtx C) D { return metaGlyph(gtx, th, icPullCheck, i18n.T("pulls.reviewThread.resolved"), th.TonePass) @@ -296,7 +306,7 @@ func (v *pullView) threadBody(gtx C, th *ui.Theme, p *pullsScreen, d backend.PRD } if !editing { rows = append(rows, layout.Rigid(func(gtx C) D { - return layout.Inset{Top: ui.Space(2.5)}.Layout(gtx, func(gtx C) D { return v.threadActions(gtx, th, d, t, act) }) + return layout.Inset{Top: ui.Space(2.5)}.Layout(gtx, func(gtx C) D { return v.threadActions(gtx, th, p, d, t, act) }) })) } return layout.Flex{Axis: layout.Vertical}.Layout(gtx, rows...) @@ -332,8 +342,9 @@ func hunkTail(gtx C, th *ui.Theme, hunk string) D { return layout.Flex{Axis: layout.Vertical}.Layout(gtx, children...) } -// threadActions is Reply and Resolve, or the reply box while one is written. -func (v *pullView) threadActions(gtx C, th *ui.Theme, d backend.PRDetail, t backend.PRReviewThread, act *pullsAction) D { +// threadActions is Reply and Resolve, with Send to session at the end, or +// the reply box while one is written. +func (v *pullView) threadActions(gtx C, th *ui.Theme, p *pullsScreen, d backend.PRDetail, t backend.PRReviewThread, act *pullsAction) D { s := &v.talk reply, resolve := clickableOf(s.replyClicks, t.ID), clickableOf(s.resolveClick, t.ID) if reply.Clicked(gtx) { @@ -359,7 +370,47 @@ func (v *pullView) threadActions(gtx C, th *ui.Theme, d backend.PRDetail, t back resolveB := th.Button(resolve, label) resolveB.Variant, resolveB.Size, resolveB.Disabled = ui.VariantGhost, ui.SizeXS, busy == "resolve:"+t.ID return layout.Flex{Alignment: layout.Middle}.Layout(gtx, - layout.Rigid(replyB.Layout), ui.Gap(2), layout.Rigid(resolveB.Layout)) + layout.Rigid(replyB.Layout), ui.Gap(2), layout.Rigid(resolveB.Layout), + layout.Flexed(1, func(gtx C) D { return D{Size: image.Pt(gtx.Constraints.Min.X, 0)} }), + layout.Rigid(func(gtx C) D { return v.toSessionButton(gtx, th, p, d, t, act) })) +} + +// toSessionButton is Send to session, Open in session while the branch's +// checkout has no session; nothing on a thread that cannot go. +func (v *pullView) toSessionButton(gtx C, th *ui.Theme, p *pullsScreen, d backend.PRDetail, t backend.PRReviewThread, act *pullsAction) D { + if !canSendThreads(d) || !p.threadSendable(t) { + return D{} + } + click := clickableOf(v.talk.toSession, t.ID) + busy := v.busyNow() == "threads" + if click.Clicked(gtx) && !busy { + act.threads = []backend.PRReviewThread{t} + } + label, tip := i18n.T("pulls.pulls.openInSession"), "" + if id, _ := p.reviewSession(d); id != "" { + label, tip = i18n.T("pulls.threadSend.send"), i18n.T("pulls.threadSend.sendTip", i18n.Args{"name": p.sessionLabel(id)}) + } + b := th.Button(click, label) + b.Icon, b.Variant, b.Size, b.Disabled = icPullSession, ui.VariantGhost, ui.SizeXS, busy + return v.withTip(gtx, th, p, "toSession:"+t.ID, tip, b.Layout) +} + +// sendOpenThreads is Send open threads, above the timeline: every open thread +// not yet waiting, in one prompt. +func (v *pullView) sendOpenThreads(th *ui.Theme, p *pullsScreen, d backend.PRDetail, threads []backend.PRReviewThread, act *pullsAction) layout.Widget { + return func(gtx C) D { + busy := v.busyNow() == "threads" + if v.talk.allToSession.Clicked(gtx) && !busy { + act.threads = threads + } + b := th.Button(&v.talk.allToSession, i18n.T("pulls.threadSend.sendAll", i18n.Args{"count": len(threads)})) + b.Icon, b.Variant, b.Size, b.Disabled = icPullSession, ui.VariantOutline, ui.SizeXS, busy + return layout.Inset{Left: pullsPadX, Right: pullsPadX, Top: ui.Space(3)}.Layout(gtx, func(gtx C) D { + return layout.Flex{Alignment: layout.Middle}.Layout(gtx, + layout.Flexed(1, func(gtx C) D { return D{Size: image.Pt(gtx.Constraints.Min.X, 0)} }), + layout.Rigid(b.Layout)) + }) + } } func (v *pullView) replyBox(gtx C, th *ui.Theme, d backend.PRDetail, t backend.PRReviewThread, act *pullsAction) D { diff --git a/native/cmd/lich-native/pulls_threads.go b/native/cmd/lich-native/pulls_threads.go new file mode 100644 index 000000000..ebea6bc8c --- /dev/null +++ b/native/cmd/lich-native/pulls_threads.go @@ -0,0 +1,111 @@ +package main + +import ( + "fmt" + "strings" + + "github.com/lichdotdev/lich/api" + "github.com/lichdotdev/lich/native/backend" + "github.com/lichdotdev/lich/native/i18n" + "github.com/lichdotdev/lich/native/ui" +) + +// ghostAuthor is the login GitHub shows for an account deleted since. +const ghostAuthor = "ghost" + +// canSendThreads is whether the PR's threads may go to a session: it is open, +// and its branch is one lich can check out and push to. +func canSendThreads(d backend.PRDetail) bool { + return d.State == "OPEN" && (!d.IsCrossRepository || d.MaintainerCanModify) +} + +// threadWaiting is whether thread id waits for the push that resolves it. +func (p *pullsScreen) threadWaiting(id string) bool { return p.waiting != nil && p.waiting(id) } + +// threadSendable is whether thread t can go to a session: open, not already +// waiting for its push, and with a comment for the replies to hang off. +func (p *pullsScreen) threadSendable(t backend.PRReviewThread) bool { + return !t.IsResolved && len(t.Comments) > 0 && !p.threadWaiting(t.ID) +} + +// sendableThreads is the threads Send open threads hands over. +func (p *pullsScreen) sendableThreads(c *backend.PRConversation) []backend.PRReviewThread { + if c == nil { + return nil + } + var out []backend.PRReviewThread + for _, t := range c.Threads { + if p.threadSendable(t) { + out = append(out, t) + } + } + return out +} + +// threadsPrompt is what a session gets for threads: each one where it is and +// what was said in it, then the ask. +func threadsPrompt(d backend.PRDetail, threads []backend.PRReviewThread) string { + parts := make([]string, 0, len(threads)+1) + for _, t := range threads { + parts = append(parts, threadBlock(d, t)) + } + parts = append(parts, i18n.Prompt("prompts.reviewThread.ask", i18n.Args{"count": len(threads)})) + return strings.Join(parts, "\n\n") +} + +func threadBlock(d backend.PRDetail, t backend.PRReviewThread) string { + args := i18n.Args{"number": d.Number, "branch": d.HeadRefName, "path": t.Path, "lines": threadLines(t)} + head := i18n.Prompt("prompts.reviewThread.head", args) + if t.IsOutdated { + head = i18n.Prompt("prompts.reviewThread.headOutdated", args) + } + lines := []string{head} + if d.URL != "" && len(t.Comments) > 0 { + lines = append(lines, fmt.Sprintf("%s#discussion_r%d", d.URL, t.Comments[0].ID)) + } + lines = append(lines, "") + for _, c := range t.Comments { + author := c.Author + if author == "" { + author = ghostAuthor + } + lines = append(lines, "- @"+author+": "+continuation(strings.TrimSpace(c.Body))) + } + return strings.Join(lines, "\n") +} + +// threadLines is the line ("19") or the span ("12-30") a thread sits on. +func threadLines(t backend.PRReviewThread) string { + if t.StartLine > 0 && t.StartLine != t.Line { + return fmt.Sprintf("%d-%d", t.StartLine, t.Line) + } + return fmt.Sprint(t.Line) +} + +// sendThreads hands threads to the PR's session, opening one when its checkout +// has none, and, once the session took the prompt, has each answered on +// GitHub and waiting for the push that resolves it. +func (u *window) sendThreads(project backend.Project, d backend.PRDetail, threads []backend.PRReviewThread) { + u.pulls.view.setBusy("threads") + defer u.pulls.view.setBusy("") + id, err := u.prSession(project, d) + if err != nil { + u.toaster.Show(ui.Toast{Variant: ui.ToastError, Title: i18n.T("pulls.pulls.openSessionFailed", i18n.Args{"error": backend.ErrorText(err)})}) + return + } + if !u.awaitPrompt(id) { + return + } + if err := u.client.Submit(u.ctx, id, threadsPrompt(d, threads)); err != nil { + u.toaster.Show(ui.Toast{Variant: ui.ToastError, Title: i18n.T("sidebar.worktreeDialog.handoffLost", i18n.Args{"error": backend.ErrorText(err)})}) + return + } + for _, t := range threads { + opts := api.ThreadFixOptions{Path: project.Path, Number: d.Number, ThreadID: t.ID, CommentID: t.Comments[len(t.Comments)-1].ID} + if err := u.client.AwaitThreadFix(u.ctx, opts); err != nil { + u.toaster.Show(ui.Toast{Variant: ui.ToastError, Title: i18n.T("pulls.threadSend.awaitFailed", i18n.Args{"error": backend.ErrorText(err)})}) + return + } + } + u.toaster.Show(ui.Toast{Variant: ui.ToastSuccess, Title: i18n.T("pulls.threadSend.sent", i18n.Args{"count": len(threads), "name": u.model.sessionLabel(id)})}) +} diff --git a/native/cmd/lich-native/pulls_threads_test.go b/native/cmd/lich-native/pulls_threads_test.go new file mode 100644 index 000000000..225cc7bfd --- /dev/null +++ b/native/cmd/lich-native/pulls_threads_test.go @@ -0,0 +1,68 @@ +package main + +import ( + "testing" + + "github.com/lichdotdev/lich/native/backend" +) + +func TestThreadsPromptNamesWhereAndWhatWasSaid(t *testing.T) { + d := backend.PRDetail{Number: 933, HeadRefName: "fix/kiro-paste", URL: "https://github.com/o/r/pull/933"} + threads := []backend.PRReviewThread{ + {Path: "internal/relay/submit.go", StartLine: 41, Line: 58, Comments: []backend.PRThreadComment{ + {ID: 2201, Author: "reviewer", Body: "Wait for the echo?\nOnly on Kiro."}, + {ID: 2202, Body: "Agreed."}, + }}, + {Path: "pulls.go", Line: 112, IsOutdated: true, Comments: []backend.PRThreadComment{{ID: 3301, Author: "reviewer", Body: "Duplicated rule."}}}, + } + want := "Review thread on pull request #933 (fix/kiro-paste), internal/relay/submit.go:41-58:\n" + + "https://github.com/o/r/pull/933#discussion_r2201\n\n" + + "- @reviewer: Wait for the echo?\n Only on Kiro.\n" + + "- @ghost: Agreed.\n\n" + + "Review thread on pull request #933 (fix/kiro-paste), pulls.go:112 (outdated, check the current code):\n" + + "https://github.com/o/r/pull/933#discussion_r3301\n\n" + + "- @reviewer: Duplicated rule.\n\n" + + "Address them and commit the fixes. lich replies to and resolves the threads on GitHub once the fixes are pushed, so don't." + if got := threadsPrompt(d, threads); got != want { + t.Errorf("threadsPrompt =\n%s\nwant\n%s", got, want) + } + one := threadsPrompt(d, threads[1:]) + if wantTail := "Address it and commit the fix. lich replies to and resolves the thread on GitHub once the fix is pushed, so don't."; one[len(one)-len(wantTail):] != wantTail { + t.Errorf("one thread's ask = %q", one) + } +} + +func TestOnlyOpenUnsentThreadsGo(t *testing.T) { + p := &pullsScreen{waiting: func(id string) bool { return id == "sent" }} + said := []backend.PRThreadComment{{ID: 1}} + conv := &backend.PRConversation{Threads: []backend.PRReviewThread{ + {ID: "open", Comments: said}, + {ID: "resolved", IsResolved: true, Comments: said}, + {ID: "sent", Comments: said}, + {ID: "empty"}, + }} + got := p.sendableThreads(conv) + if len(got) != 1 || got[0].ID != "open" { + t.Errorf("sendableThreads = %+v, want only the open one", got) + } + if len((&pullsScreen{}).sendableThreads(conv)) != 2 { + t.Error("with nothing waiting, every open thread with a comment goes") + } +} + +func TestCanSendThreadsNeedsAnOpenPushableBranch(t *testing.T) { + cases := []struct { + d backend.PRDetail + want bool + }{ + {backend.PRDetail{State: "OPEN"}, true}, + {backend.PRDetail{State: "MERGED"}, false}, + {backend.PRDetail{State: "OPEN", IsCrossRepository: true}, false}, + {backend.PRDetail{State: "OPEN", IsCrossRepository: true, MaintainerCanModify: true}, true}, + } + for _, c := range cases { + if got := canSendThreads(c.d); got != c.want { + t.Errorf("canSendThreads(%+v) = %v, want %v", c.d, got, c.want) + } + } +} diff --git a/native/cmd/lich-native/pulls_view.go b/native/cmd/lich-native/pulls_view.go index 1e2765cb4..890c27705 100644 --- a/native/cmd/lich-native/pulls_view.go +++ b/native/cmd/lich-native/pulls_view.go @@ -45,6 +45,7 @@ type pullsAction struct { comment string // a comment on the pull request itself reply *threadReply resolve *threadResolve + threads []backend.PRReviewThread // review threads for the PR's session edit *commentEdit title string // a new title for the pull request body *string // a new description; empty clears it diff --git a/native/cmd/lich-native/threadfix_store.go b/native/cmd/lich-native/threadfix_store.go new file mode 100644 index 000000000..7e6a0b727 --- /dev/null +++ b/native/cmd/lich-native/threadfix_store.go @@ -0,0 +1,42 @@ +package main + +import ( + "context" + "sync" + + "github.com/lichdotdev/lich/native/backend" +) + +// threadFixStore is the review threads waiting for their pull request's next +// push, as the backend last announced them (api.ThreadFixesEvent): the Pulls +// screen reads it, and only the event writes it. +type threadFixStore struct { + mu sync.Mutex + waiting map[string]bool +} + +func (s *threadFixStore) set(list []backend.ThreadFix) { + waiting := make(map[string]bool, len(list)) + for _, f := range list { + waiting[f.ThreadID] = true + } + s.mu.Lock() + s.waiting = waiting + s.mu.Unlock() +} + +func (s *threadFixStore) load(ctx context.Context, client *backend.Client) error { + list, err := client.ThreadFixes(ctx) + if err != nil { + return err + } + s.set(list) + return nil +} + +// has is whether thread id waits for a push. +func (s *threadFixStore) has(id string) bool { + s.mu.Lock() + defer s.mu.Unlock() + return s.waiting[id] +} diff --git a/native/i18n/locales/en/prompts.json b/native/i18n/locales/en/prompts.json index c862c9315..56504691a 100644 --- a/native/i18n/locales/en/prompts.json +++ b/native/i18n/locales/en/prompts.json @@ -14,6 +14,14 @@ "reviewComments": { "header": "Review comments:" }, + "reviewThread": { + "head": "Review thread on pull request #{number} ({branch}), {path}:{lines}:", + "headOutdated": "Review thread on pull request #{number} ({branch}), {path}:{lines} (outdated, check the current code):", + "ask": { + "one": "Address it and commit the fix. lich replies to and resolves the thread on GitHub once the fix is pushed, so don't.", + "other": "Address them and commit the fixes. lich replies to and resolves the threads on GitHub once the fixes are pushed, so don't." + } + }, "issue": { "head": "GitHub issue #{number} — {title}\n{url}" } diff --git a/native/i18n/locales/en/pulls.json b/native/i18n/locales/en/pulls.json index 44594f614..284dc71be 100644 --- a/native/i18n/locales/en/pulls.json +++ b/native/i18n/locales/en/pulls.json @@ -195,6 +195,20 @@ "verdictStays": "The verdict stays as it is.", "more": "Comment options" }, + "threadSend": { + "send": "Send to session", + "sendTip": "Sends this thread to {name}", + "sendAll": { + "one": "Send {count} open thread to session", + "other": "Send {count} open threads to session" + }, + "waiting": "Sent · waiting for a push", + "sent": { + "one": "Sent the thread to {name}", + "other": "Sent {count} threads to {name}" + }, + "awaitFailed": "The session has the threads, but GitHub wasn't updated: {error}" + }, "pulls": { "alreadyRemoved": "{name} was already removed", "pinnedSession": "Worktree has a pinned session — unpin it first.", diff --git a/native/i18n/locales/es/prompts.json b/native/i18n/locales/es/prompts.json index 231bcbc0a..1151e142e 100644 --- a/native/i18n/locales/es/prompts.json +++ b/native/i18n/locales/es/prompts.json @@ -14,6 +14,14 @@ "reviewComments": { "header": "Comentarios de revisión:" }, + "reviewThread": { + "head": "Hilo de revisión en el pull request #{number} ({branch}), {path}:{lines}:", + "headOutdated": "Hilo de revisión en el pull request #{number} ({branch}), {path}:{lines} (desactualizado, revisa el código actual):", + "ask": { + "one": "Resuélvelo y haz commit de la corrección. lich responde y resuelve el hilo en GitHub cuando la corrección llegue con un push, así que no lo hagas tú.", + "other": "Resuélvelos y haz commit de las correcciones. lich responde y resuelve los hilos en GitHub cuando las correcciones lleguen con un push, así que no lo hagas tú." + } + }, "issue": { "head": "Issue de GitHub #{number}: {title}\n{url}" } diff --git a/native/i18n/locales/es/pulls.json b/native/i18n/locales/es/pulls.json index 6fe309778..6272df59f 100644 --- a/native/i18n/locales/es/pulls.json +++ b/native/i18n/locales/es/pulls.json @@ -195,6 +195,20 @@ "verdictStays": "El veredicto no cambia.", "more": "Opciones del comentario" }, + "threadSend": { + "send": "Enviar a la sesión", + "sendTip": "Envía este hilo a {name}", + "sendAll": { + "one": "Enviar {count} hilo abierto a la sesión", + "other": "Enviar {count} hilos abiertos a la sesión" + }, + "waiting": "Enviado · esperando un push", + "sent": { + "one": "Hilo enviado a {name}", + "other": "{count} hilos enviados a {name}" + }, + "awaitFailed": "La sesión recibió los hilos, pero GitHub no se actualizó: {error}" + }, "pulls": { "alreadyRemoved": "{name} ya se había eliminado", "pinnedSession": "La worktree tiene una sesión fijada: desfíjala primero.", diff --git a/native/i18n/locales/ja/prompts.json b/native/i18n/locales/ja/prompts.json index 12e8ec54c..3afb4b752 100644 --- a/native/i18n/locales/ja/prompts.json +++ b/native/i18n/locales/ja/prompts.json @@ -14,6 +14,14 @@ "reviewComments": { "header": "レビューコメント:" }, + "reviewThread": { + "head": "pull request #{number} ({branch}) のレビュースレッド、{path}:{lines}:", + "headOutdated": "pull request #{number} ({branch}) のレビュースレッド、{path}:{lines}（古いコメントです。現在のコードを確認してください）:", + "ask": { + "one": "対応して修正をコミットしてください。修正が push されると lich が GitHub でスレッドに返信して解決済みにするので、自分では行わないでください。", + "other": "すべてに対応して修正をコミットしてください。修正が push されると lich が GitHub で各スレッドに返信して解決済みにするので、自分では行わないでください。" + } + }, "issue": { "head": "GitHub issue #{number}: {title}\n{url}" } diff --git a/native/i18n/locales/ja/pulls.json b/native/i18n/locales/ja/pulls.json index 0b5bc7157..8e8b59eec 100644 --- a/native/i18n/locales/ja/pulls.json +++ b/native/i18n/locales/ja/pulls.json @@ -194,6 +194,20 @@ "verdictStays": "判定は変わりません。", "more": "コメントのオプション" }, + "threadSend": { + "send": "セッションに送る", + "sendTip": "このスレッドを {name} に送ります", + "sendAll": { + "one": "未解決のスレッド {count} 件をセッションに送る", + "other": "未解決のスレッド {count} 件をセッションに送る" + }, + "waiting": "送信済み · push 待ち", + "sent": { + "one": "スレッドを {name} に送りました", + "other": "スレッド {count} 件を {name} に送りました" + }, + "awaitFailed": "スレッドはセッションに届きましたが、GitHub を更新できませんでした: {error}" + }, "pulls": { "alreadyRemoved": "{name} はすでに削除されています", "pinnedSession": "ワークツリーにピン留めされたセッションがあります。先にピン留めを外してください。", diff --git a/native/i18n/locales/ko/prompts.json b/native/i18n/locales/ko/prompts.json index dcb17d5d6..869c16352 100644 --- a/native/i18n/locales/ko/prompts.json +++ b/native/i18n/locales/ko/prompts.json @@ -12,6 +12,14 @@ "reviewComments": { "header": "리뷰 코멘트:" }, + "reviewThread": { + "head": "pull request #{number} ({branch})의 리뷰 스레드, {path}:{lines}:", + "headOutdated": "pull request #{number} ({branch})의 리뷰 스레드, {path}:{lines} (오래된 코멘트이니 현재 코드를 확인하세요):", + "ask": { + "one": "처리하고 수정 사항을 커밋하세요. 수정이 push되면 lich가 GitHub에서 스레드에 답글을 달고 해결 처리하므로 직접 하지 마세요.", + "other": "모두 처리하고 수정 사항을 커밋하세요. 수정이 push되면 lich가 GitHub에서 각 스레드에 답글을 달고 해결 처리하므로 직접 하지 마세요." + } + }, "issue": { "head": "GitHub 이슈 #{number}: {title}\n{url}" } diff --git a/native/i18n/locales/ko/pulls.json b/native/i18n/locales/ko/pulls.json index 8546d8f10..566e63eef 100644 --- a/native/i18n/locales/ko/pulls.json +++ b/native/i18n/locales/ko/pulls.json @@ -190,6 +190,20 @@ "verdictStays": "판정은 그대로 유지됩니다.", "more": "댓글 옵션" }, + "threadSend": { + "send": "세션으로 보내기", + "sendTip": "이 스레드를 {name}(으)로 보냅니다", + "sendAll": { + "one": "열린 스레드 {count}개를 세션으로 보내기", + "other": "열린 스레드 {count}개를 세션으로 보내기" + }, + "waiting": "보냄 · push 대기 중", + "sent": { + "one": "스레드를 {name}(으)로 보냈습니다", + "other": "스레드 {count}개를 {name}(으)로 보냈습니다" + }, + "awaitFailed": "세션은 스레드를 받았지만 GitHub를 업데이트하지 못했습니다: {error}" + }, "pulls": { "alreadyRemoved": "{name}은(는) 이미 삭제되었습니다", "pinnedSession": "worktree에 고정된 세션이 있습니다. 먼저 고정을 해제하세요.", diff --git a/native/i18n/locales/pt-br/prompts.json b/native/i18n/locales/pt-br/prompts.json index 36a590172..776e9e107 100644 --- a/native/i18n/locales/pt-br/prompts.json +++ b/native/i18n/locales/pt-br/prompts.json @@ -14,6 +14,14 @@ "reviewComments": { "header": "Comentários de revisão:" }, + "reviewThread": { + "head": "Thread de revisão no pull request #{number} ({branch}), {path}:{lines}:", + "headOutdated": "Thread de revisão no pull request #{number} ({branch}), {path}:{lines} (desatualizada, confira o código atual):", + "ask": { + "one": "Resolva e faça commit da correção. O lich responde e resolve a thread no GitHub quando a correção chegar por push, então não faça isso.", + "other": "Resolva todas e faça commit das correções. O lich responde e resolve as threads no GitHub quando as correções chegarem por push, então não faça isso." + } + }, "issue": { "head": "Issue do GitHub #{number}: {title}\n{url}" } diff --git a/native/i18n/locales/pt-br/pulls.json b/native/i18n/locales/pt-br/pulls.json index 4b1df8283..96ab92eae 100644 --- a/native/i18n/locales/pt-br/pulls.json +++ b/native/i18n/locales/pt-br/pulls.json @@ -195,6 +195,20 @@ "verdictStays": "O veredito continua o mesmo.", "more": "Opções do comentário" }, + "threadSend": { + "send": "Enviar para a sessão", + "sendTip": "Envia esta thread para {name}", + "sendAll": { + "one": "Enviar {count} thread aberta para a sessão", + "other": "Enviar {count} threads abertas para a sessão" + }, + "waiting": "Enviada · aguardando um push", + "sent": { + "one": "Thread enviada para {name}", + "other": "{count} threads enviadas para {name}" + }, + "awaitFailed": "A sessão recebeu as threads, mas o GitHub não foi atualizado: {error}" + }, "pulls": { "alreadyRemoved": "{name} já foi removido", "pinnedSession": "A worktree tem uma sessão fixada. Desafixe-a primeiro.", diff --git a/native/i18n/locales/ru/prompts.json b/native/i18n/locales/ru/prompts.json index fcfc1bed0..5136bcf5f 100644 --- a/native/i18n/locales/ru/prompts.json +++ b/native/i18n/locales/ru/prompts.json @@ -18,6 +18,16 @@ "reviewComments": { "header": "Комментарии ревью:" }, + "reviewThread": { + "head": "Обсуждение ревью в pull request #{number} ({branch}), {path}:{lines}:", + "headOutdated": "Обсуждение ревью в pull request #{number} ({branch}), {path}:{lines} (устарело, сверьтесь с текущим кодом):", + "ask": { + "one": "Исправьте замечание и закоммитьте исправление. lich сам ответит в обсуждении и отметит его решённым на GitHub, когда исправление попадёт в push, поэтому не делайте этого.", + "few": "Исправьте замечания и закоммитьте исправления. lich сам ответит в обсуждениях и отметит их решёнными на GitHub, когда исправления попадут в push, поэтому не делайте этого.", + "many": "Исправьте замечания и закоммитьте исправления. lich сам ответит в обсуждениях и отметит их решёнными на GitHub, когда исправления попадут в push, поэтому не делайте этого.", + "other": "Исправьте замечания и закоммитьте исправления. lich сам ответит в обсуждениях и отметит их решёнными на GitHub, когда исправления попадут в push, поэтому не делайте этого." + } + }, "issue": { "head": "GitHub issue #{number}: {title}\n{url}" } diff --git a/native/i18n/locales/ru/pulls.json b/native/i18n/locales/ru/pulls.json index 5c715a46e..ece92eccb 100644 --- a/native/i18n/locales/ru/pulls.json +++ b/native/i18n/locales/ru/pulls.json @@ -205,6 +205,24 @@ "verdictStays": "Вердикт останется прежним.", "more": "Действия с комментарием" }, + "threadSend": { + "send": "Отправить в сессию", + "sendTip": "Отправляет это обсуждение в «{name}»", + "sendAll": { + "one": "Отправить {count} открытое обсуждение в сессию", + "few": "Отправить {count} открытых обсуждения в сессию", + "many": "Отправить {count} открытых обсуждений в сессию", + "other": "Отправить {count} открытого обсуждения в сессию" + }, + "waiting": "Отправлено · ждёт push", + "sent": { + "one": "Обсуждение отправлено в «{name}»", + "few": "{count} обсуждения отправлены в «{name}»", + "many": "{count} обсуждений отправлено в «{name}»", + "other": "{count} обсуждения отправлено в «{name}»" + }, + "awaitFailed": "Сессия получила обсуждения, но GitHub не обновлён: {error}" + }, "pulls": { "alreadyRemoved": "{name} уже удалён", "pinnedSession": "В worktree есть закреплённая сессия: сначала открепите её.", diff --git a/native/i18n/locales/zh-cn/prompts.json b/native/i18n/locales/zh-cn/prompts.json index 44b116f3a..c4e7f54b9 100644 --- a/native/i18n/locales/zh-cn/prompts.json +++ b/native/i18n/locales/zh-cn/prompts.json @@ -14,6 +14,14 @@ "reviewComments": { "header": "审查评论：" }, + "reviewThread": { + "head": "pull request #{number} ({branch}) 的审查讨论，{path}:{lines}：", + "headOutdated": "pull request #{number} ({branch}) 的审查讨论，{path}:{lines}（已过时，请对照当前代码）：", + "ask": { + "one": "请处理并提交修复。修复 push 后，lich 会在 GitHub 上回复该讨论并标记为已解决，所以你不要这样做。", + "other": "请逐一处理并提交修复。修复 push 后，lich 会在 GitHub 上回复这些讨论并标记为已解决，所以你不要这样做。" + } + }, "issue": { "head": "GitHub issue #{number}，{title}\n{url}" } diff --git a/native/i18n/locales/zh-cn/pulls.json b/native/i18n/locales/zh-cn/pulls.json index 44a2ea49e..fc8f4468c 100644 --- a/native/i18n/locales/zh-cn/pulls.json +++ b/native/i18n/locales/zh-cn/pulls.json @@ -194,6 +194,20 @@ "verdictStays": "结论保持不变。", "more": "评论选项" }, + "threadSend": { + "send": "发送到会话", + "sendTip": "将此讨论发送到 {name}", + "sendAll": { + "one": "将 {count} 个未解决的讨论发送到会话", + "other": "将 {count} 个未解决的讨论发送到会话" + }, + "waiting": "已发送 · 等待 push", + "sent": { + "one": "已将讨论发送到 {name}", + "other": "已将 {count} 个讨论发送到 {name}" + }, + "awaitFailed": "会话已收到讨论，但未能更新 GitHub：{error}" + }, "pulls": { "alreadyRemoved": "{name} 已被移除", "pinnedSession": "该工作树有已固定的会话，请先取消固定。",