From e10e0d22a9f928a6199439b5ec0329f6950caf71 Mon Sep 17 00:00:00 2001 From: omartelo Date: Thu, 22 Oct 2026 17:10:40 -0300 Subject: [PATCH] fix(native): send a pull request's review comments to its own session The Files tab in Pulls bound its comment batch and line injects to the window's active session and keyed the batch by that session's checkout, so Send typed a pull request's comments into whichever session was open before Pulls, reviewing another branch. reviewSession now picks a session in the checkout of the pull request's head branch: the active one when it sits there, the first otherwise. With none, Send refuses and keeps the batch. A branch with no checkout keeps its batch under the pull request's page, and the comments move into the checkout once Open in Session creates it. --- CHANGELOG.md | 1 + native/cmd/lich-native/dock_comments.go | 15 +++++++++ native/cmd/lich-native/dock_comments_test.go | 18 +++++++++++ native/cmd/lich-native/main.go | 6 ++-- native/cmd/lich-native/pulls.go | 26 ++++++++++++--- native/cmd/lich-native/pulls_files.go | 22 +++++++------ native/cmd/lich-native/pulls_panels.go | 2 +- native/cmd/lich-native/pulls_store_test.go | 33 ++++++++++++++++++++ native/cmd/lich-native/stale_pulls_test.go | 5 +++ 9 files changed, 110 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d7ba67bc..11ae9de0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- Comments for the session written on a pull request's Files tab in Pulls go to a session in the checkout of that pull request's branch, where Send typed them into whichever session was open before Pulls, reviewing another branch. With no session there, Send says to open one and keeps the comments; Open in Session starts it. A line reference sent from that tab goes the same way. - On Windows with display scaling above 100%, a drag follows the pointer again: an item dragged in Settings › Footer or a selection dragged in the terminal landed away from the cursor, scaled as if the screen were at 100%. - Review comments sent to a session are kept until the session takes them. A session that refused the batch (one that had stopped, say) showed "Couldn't send the comments" while the comments were already gone; they now stay in the batch to send again. Likewise, submitting a pull request review in Pulls drops only the comments it carried, where a comment written while the review was on its way was lost with them. - On Windows, the window wears lich's icon and opens under lich's taskbar button, where it showed a generic icon and, launched from a lich pinned to the taskbar, opened as a separate button beside the pin. diff --git a/native/cmd/lich-native/dock_comments.go b/native/cmd/lich-native/dock_comments.go index 5cae9abe..4a9e18f6 100644 --- a/native/cmd/lich-native/dock_comments.go +++ b/native/cmd/lich-native/dock_comments.go @@ -102,6 +102,21 @@ func (s *commentStore) edit(target string, id int, text string) { func (s *commentStore) clear(target string) { delete(s.byTarget, target) } +// move hands from's batch to to, after what to already holds: a pull +// request's comments, kept under its page while no checkout held its branch, +// follow it into the one that now does. +func (s *commentStore) move(from, to string) { + moved, ok := s.byTarget[from] + if !ok { + return + } + s.byTarget[to] = append(s.byTarget[to], moved...) + delete(s.byTarget, from) + if s.editing.target == from { + s.editing.target = to + } +} + func (s *commentStore) beginEdit(target string, id int) { s.editing = commentRef{target, id} } func (s *commentStore) endEdit() { s.editing = commentRef{} } diff --git a/native/cmd/lich-native/dock_comments_test.go b/native/cmd/lich-native/dock_comments_test.go index cf2c2550..29e64862 100644 --- a/native/cmd/lich-native/dock_comments_test.go +++ b/native/cmd/lich-native/dock_comments_test.go @@ -131,3 +131,21 @@ func TestCommentEditFollowsTheCommentItWasOpenedOn(t *testing.T) { t.Fatal("still editing after endEdit") } } + +func TestCommentBatchMovesWholeAfterWhatItJoins(t *testing.T) { + var s commentStore + s.add("/wt", "a.go", "1", "already here") + s.add("https://pr/1", "b.go", "2", "written before the checkout") + s.beginEdit("https://pr/1", s.list("https://pr/1")[0].id) + s.move("https://pr/1", "/wt") + got := s.list("/wt") + if len(got) != 2 || got[0].txt != "already here" || got[1].txt != "written before the checkout" { + t.Fatalf("batch of /wt = %+v, want its own comment then the moved one", got) + } + if len(s.list("https://pr/1")) != 0 { + t.Fatalf("batch left behind = %+v, want none", s.list("https://pr/1")) + } + if c, ok := s.edited(); !ok || c.txt != "written before the checkout" { + t.Fatalf("edited = %+v, %v; want the moved comment still open", c, ok) + } +} diff --git a/native/cmd/lich-native/main.go b/native/cmd/lich-native/main.go index f3dd7331..86dc5964 100644 --- a/native/cmd/lich-native/main.go +++ b/native/cmd/lich-native/main.go @@ -220,9 +220,9 @@ func run(w *app.Window, rt lichclient.Runtime) error { u.palette = newCommandPalette(w.Invalidate) u.worktree = newWorktreeDialog(client, &u.root, w.Invalidate) u.pulls = newPullsScreen(newPullsStore(ctx, client, w.Invalidate), &u.root) - u.pulls.sessionAt = func(projectID, path string) bool { + u.pulls.sessionsIn = func(projectID, path string) []string { ids, _, _ := u.model.sessionsAt(projectID, path) - return len(ids) > 0 + return ids } u.model.prChanged = u.prChanged u.model.themeSelection = u.appearance.applySelection @@ -600,7 +600,7 @@ func (u *window) body(gtx C, v stageView) (int, sidebarAction, error) { return D{Size: gtx.Constraints.Max} } if u.pulls.open && !v.home { - u.pulls.files = filesInput{sessionID: v.in.sessionID, inject: u.terms.injector(v.in.sessionID), submit: u.terms.submitter(v.in.sessionID), checkout: v.in.path, light: u.light, code: u.code} + u.pulls.files = filesInput{activeID: v.in.sessionID, injector: u.terms.injector, submitter: u.terms.submitter, light: u.light, code: u.code} u.pulls.squareX = -1 if x, ok := u.topbar.islandSquare(gtx, topbarFrame{tabs: v.strip, active: v.stripActive, islands: u.pulls.spans, islandsSliding: u.pulls.slide.sliding()}); ok { u.pulls.squareX = x diff --git a/native/cmd/lich-native/pulls.go b/native/cmd/lich-native/pulls.go index 07a4c636..cc9a43b4 100644 --- a/native/cmd/lich-native/pulls.go +++ b/native/cmd/lich-native/pulls.go @@ -3,6 +3,7 @@ package main import ( "image" "image/color" + "slices" "time" "gioui.org/layout" @@ -98,14 +99,29 @@ type pullsScreen struct { view pullView confirm removeConfirm - // files is the window's active session and checkout, for the Files tab. + // files is what the Files tab draws with and sends through. files filesInput - // sessionAt is whether a session of the project sits in a checkout. - sessionAt func(projectID, path string) bool + // sessionsIn is the project's sessions sitting in a checkout. + sessionsIn func(projectID, path string) []string } -func (p *pullsScreen) hasSessionAt(path string) bool { - return p.sessionAt != nil && p.sessionAt(p.project.ID, path) +// reviewSession is where the pull request's comments go: a session in the +// checkout of its head branch, the window's active one when it sits there and +// the first otherwise, "" when none does. target keys its comment batch: that checkout, shared with its +// dock, or the PR's page while the branch has none. +func (p *pullsScreen) reviewSession(d lichclient.PRDetail) (id, target string) { + wt, ok := p.checkedOut(d.HeadRefName) + if !ok { + return "", d.URL + } + ids := p.sessionsIn(p.project.ID, wt.Path) + switch { + case slices.Contains(ids, p.files.activeID): + return p.files.activeID, wt.Path + case len(ids) > 0: + return ids[0], wt.Path + } + return "", wt.Path } func newPullsScreen(store *pullsStore, root *ui.Root) *pullsScreen { diff --git a/native/cmd/lich-native/pulls_files.go b/native/cmd/lich-native/pulls_files.go index b995fae7..99083692 100644 --- a/native/cmd/lich-native/pulls_files.go +++ b/native/cmd/lich-native/pulls_files.go @@ -64,14 +64,14 @@ type filesTree struct { } // filesInput is what the Files tab needs from the window this frame: the -// active session, whose terminal takes injected references, and its -// checkout, whose comment batch the tab shares with the dock. +// active session, and how to inject into and submit to a session, bound to +// the PR's own (reviewSession). type filesInput struct { - sessionID, checkout string - inject func(string) bool - submit func(string, func(error)) bool - light bool - code godemirror.Theme + activeID string + injector func(id string) func(string) bool + submitter func(id string) func(string, func(error)) bool + light bool + code godemirror.Theme } // viewedTick is the card's Viewed checkbox; ticking folds the file away. @@ -132,8 +132,12 @@ func (v *pullView) filesTab(gtx C, th *ui.Theme, p *pullsScreen, d lichclient.PR files.pr.head = d.Commits[len(d.Commits)-1].OID } in := p.files - files.th, files.light, files.code, files.target = th, in.light, in.code, in.checkout - files.inject, files.submit = in.inject, in.submit + session, target := p.reviewSession(d) + if target != d.URL { + files.comments.move(d.URL, target) + } + files.th, files.light, files.code, files.target = th, in.light, in.code, target + files.inject, files.submit = in.injector(session), in.submitter(session) read := p.store.diff(p.project.Path, d.Number) switch { case !read.loaded: diff --git a/native/cmd/lich-native/pulls_panels.go b/native/cmd/lich-native/pulls_panels.go index 85c9f5bf..5cd27fe8 100644 --- a/native/cmd/lich-native/pulls_panels.go +++ b/native/cmd/lich-native/pulls_panels.go @@ -68,7 +68,7 @@ func (v *pullView) actions(gtx C, th *ui.Theme, p *pullsScreen, d lichclient.PRD // sits in the branch's checkout. func (v *pullView) sessionButton(gtx C, th *ui.Theme, p *pullsScreen, d lichclient.PRDetail, busy, blocked string) D { label := i18n.T("pulls.pulls.openInSession") - if wt, ok := p.checkedOut(d.HeadRefName); ok && p.hasSessionAt(wt.Path) { + if id, _ := p.reviewSession(d); id != "" { label = i18n.T("pulls.pulls.goToSession") } if busy == "session" { diff --git a/native/cmd/lich-native/pulls_store_test.go b/native/cmd/lich-native/pulls_store_test.go index 7489ddc4..fbf5068e 100644 --- a/native/cmd/lich-native/pulls_store_test.go +++ b/native/cmd/lich-native/pulls_store_test.go @@ -109,3 +109,36 @@ func TestPullsScreenRereadsWhatItShowsEveryRecheck(t *testing.T) { p.store.list("/repo", pullsOpen) waitFor(t, "the recheck", func() bool { return stub.calls("project.ListPullRequests") == 2 }) } + +func TestPullRequestCommentsGoOnlyToASessionInItsBranchCheckout(t *testing.T) { + stub := &rpcStub{answer: func(method string, _ []json.RawMessage) (any, int) { + if method == "project.ListCheckouts" { + return []lichclient.Worktree{{Name: "main", Path: "/repo"}, {Name: "feat/x", Path: "/wt/feat-x"}}, http.StatusOK + } + return []lichclient.PRSummary{}, http.StatusOK + }} + p := newPullsScreen(newPullsStore(context.Background(), stub.client(t), func() {}), new(ui.Root)) + p.show(lichclient.Project{ID: "p", Path: "/repo"}, "main", 0) + p.sessionsIn = func(projectID, path string) []string { + if projectID == "p" && path == "/wt/feat-x" { + return []string{"in-branch", "second"} + } + return []string{"elsewhere"} + } + waitFor(t, "the checkouts", func() bool { return len(p.store.checkoutsOf("/repo")) == 2 }) + + if id, target := p.reviewSession(lichclient.PRDetail{HeadRefName: "feat/x", URL: "https://pr/1"}); id != "in-branch" || target != "/wt/feat-x" { + t.Errorf("checked-out branch: session %q, target %q; want in-branch, /wt/feat-x", id, target) + } + p.files.activeID = "second" + if id, _ := p.reviewSession(lichclient.PRDetail{HeadRefName: "feat/x"}); id != "second" { + t.Errorf("active session in the checkout: session %q, want second", id) + } + p.files.activeID = "elsewhere" + if id, _ := p.reviewSession(lichclient.PRDetail{HeadRefName: "feat/x"}); id != "in-branch" { + t.Errorf("active session in another checkout: session %q, want in-branch", id) + } + if id, target := p.reviewSession(lichclient.PRDetail{HeadRefName: "feat/y", URL: "https://pr/2"}); id != "" || target != "https://pr/2" { + t.Errorf("branch with no checkout: session %q, target %q; want none, the PR's page", id, target) + } +} diff --git a/native/cmd/lich-native/stale_pulls_test.go b/native/cmd/lich-native/stale_pulls_test.go index ee6dad51..4127d38b 100644 --- a/native/cmd/lich-native/stale_pulls_test.go +++ b/native/cmd/lich-native/stale_pulls_test.go @@ -38,6 +38,11 @@ func pullsFrames(t *testing.T) (*window, *frameRig) { client := stub.client(t) u := &window{ctx: t.Context(), client: client, model: newModel(client, r.invalidate)} u.pulls = newPullsScreen(newPullsStore(t.Context(), client, r.invalidate), &u.root) + u.pulls.sessionsIn = func(string, string) []string { return nil } + u.pulls.files = filesInput{ + injector: func(string) func(string) bool { return func(string) bool { return false } }, + submitter: func(string) func(string, func(error)) bool { return func(string, func(error)) bool { return false } }, + } files, err := newReviewPanel(client, &u.root, new(commentStore), "monospace", 12, r.invalidate, func(ui.Toast) {}) if err != nil { t.Fatal(err)