From 3a067ca8b9e9e42d93b807e8568c267a702378a1 Mon Sep 17 00:00:00 2001 From: Michael Johnson Date: Sat, 1 Aug 2026 11:02:40 +0100 Subject: [PATCH] Open a newly created PR in the browser from the TUI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Creating a PR is all but always followed by looking at it, so `g` on a review task with no tracked PR made the operator press the key twice: once to create, once to jump. Confirming the create-PR modal now chains straight into the browser with the URL `create` just recorded. The chained open is cosmetic, not part of the create: the URL is stored either way, so a browser that will not launch is reported beside the URL rather than as a failed create. `pr::create` returns the canonical URL instead of a message so the chain needs no re-read, and the browser launch is factored into `pr::open_url`, shared with the tracked-PR path. The `voro pr` CLI verb is unchanged — it formats the same message and leaves the jump to an operator who is already at a shell. --- crates/voro/src/app.rs | 83 +++++++++++++++++++++++++++++++++++++++--- crates/voro/src/cli.rs | 2 +- crates/voro/src/pr.rs | 20 +++++++--- crates/voro/src/ui.rs | 52 ++++++++++++++++++++++++++ docs/DESIGN.md | 2 +- 5 files changed, 145 insertions(+), 14 deletions(-) diff --git a/crates/voro/src/app.rs b/crates/voro/src/app.rs index f32160b..1b140ed 100644 --- a/crates/voro/src/app.rs +++ b/crates/voro/src/app.rs @@ -1460,15 +1460,13 @@ impl App { } /// Drive the create-PR confirmation modal (DESIGN.md §8). Enter (or `y`) - /// runs the same `crate::pr::create` the CLI's `pr` calls, then refreshes; - /// esc (or `n`) cancels without touching anything. + /// runs the same `crate::pr::create` the CLI's `pr` calls and shows the new + /// PR, then refreshes; esc (or `n`) cancels without touching anything. fn key_confirm_pr(&mut self, key: KeyEvent, task_id: i64, branch: String, title: String) { match key.code { KeyCode::Enter | KeyCode::Char('y') | KeyCode::Char('Y') => { - match crate::pr::create(&mut self.store, task_id) { - Ok(summary) => self.status = Some(summary), - Err(e) => self.status = Some(e), - } + let created = crate::pr::create(&mut self.store, task_id); + self.report_created_pr(task_id, created, crate::pr::open_url); let result = self.refresh(); self.report(result); } @@ -1485,6 +1483,26 @@ impl App { } } + /// Report a create-PR attempt, chaining a success straight into the browser + /// (DESIGN.md §8): creating a PR is all but always followed by looking at + /// it, so `g` does both. The create is the durable half — its URL is already + /// recorded on the task — so a browser that will not launch is reported + /// beside the URL rather than as a failed create. + fn report_created_pr( + &mut self, + task_id: i64, + created: Result, + open: impl FnOnce(&str) -> Result, + ) { + self.status = Some(match created { + Ok(url) => match open(&url) { + Ok(_) => format!("opened {url} for task {task_id} — showing it in the browser"), + Err(e) => format!("PR created ({url}); could not open browser: {e}"), + }, + Err(e) => e, + }); + } + /// Drive the link-a-PR prompt (DESIGN.md §11c). Enter validates and stores /// the reference; esc cancels. The buffer is one line — a PR URL or the /// `owner/repo#n` shorthand — so this stays a simple line editor. @@ -3800,6 +3818,59 @@ mod tests { assert!(app.store.task(task_id).unwrap().pr_url.is_none()); } + /// Confirming the create-PR modal shows the new PR without a second `g`: + /// the browser is launched with the URL `create` just recorded (DESIGN.md + /// §8). The launch is passed in so the chain is exercised without `gh`. + #[test] + fn a_created_pr_is_opened_in_the_browser() { + let mut app = app_with(&[TaskState::Review]); + let task_id = app.selected_task_id().unwrap(); + let url = "https://github.com/acme/widget/pull/7"; + let mut opened = None; + app.report_created_pr(task_id, Ok(url.to_string()), |u| { + opened = Some(u.to_string()); + Ok(format!("opening {u} in the browser")) + }); + assert_eq!(opened.as_deref(), Some(url)); + let status = app.status.as_deref().unwrap_or(""); + assert!( + status.contains(url) && status.contains("browser"), + "{status}" + ); + } + + /// A browser that will not launch does not turn a created PR into a + /// failure: the URL is recorded, so it is reported with the open error + /// alongside it (DESIGN.md §8). + #[test] + fn a_browser_failure_after_a_create_still_reports_the_pr() { + let mut app = app_with(&[TaskState::Review]); + let task_id = app.selected_task_id().unwrap(); + let url = "https://github.com/acme/widget/pull/7"; + app.report_created_pr(task_id, Ok(url.to_string()), |_| { + Err("cannot run `gh` to open the PR: not found".to_string()) + }); + let status = app.status.as_deref().unwrap_or(""); + assert!( + status.contains("PR created") && status.contains(url) && status.contains("not found"), + "{status}" + ); + } + + /// A failed create is reported as-is and never reaches the browser. + #[test] + fn a_failed_create_does_not_open_a_browser() { + let mut app = app_with(&[TaskState::Review]); + let task_id = app.selected_task_id().unwrap(); + let mut opened = false; + app.report_created_pr(task_id, Err("`gh pr create` failed".to_string()), |_| { + opened = true; + Ok(String::new()) + }); + assert!(!opened, "the browser was launched for a failed create"); + assert_eq!(app.status.as_deref(), Some("`gh pr create` failed")); + } + /// Rejecting a review task with no tracked PR opens the ordinary feedback /// prompt, empty — the pre-fill only fires when a PR is tracked (DESIGN.md /// §11c), so this path never touches `gh`. diff --git a/crates/voro/src/cli.rs b/crates/voro/src/cli.rs index e7bddbd..70d3ac8 100644 --- a/crates/voro/src/cli.rs +++ b/crates/voro/src/cli.rs @@ -1408,7 +1408,7 @@ fn pr_verb(store: &mut Store, id: i64, yes: bool, ctx: &DispatchCtx) -> Result Result { } /// Open a task's tracked PR in a browser via `gh pr view --web` (DESIGN.md -/// §11c). Spawned detached and reaped like the viewer in `dispatch::open`, so -/// nothing lingers as a zombie. +/// §11c). pub fn open(store: &Store, task_id: i64) -> Result { let pr = tracked_pr(store, task_id)?; + open_url(&pr.url) +} + +/// Open a PR URL in a browser, taking the URL rather than a task so a +/// just-created PR can be shown without re-reading the store (DESIGN.md §8). +/// Spawned detached and reaped like the viewer in `dispatch::open`, so nothing +/// lingers as a zombie. +pub fn open_url(url: &str) -> Result { let mut child = Command::new("gh") - .args(["pr", "view", "--web", &pr.url]) + .args(["pr", "view", "--web", url]) .stdin(Stdio::null()) .stdout(Stdio::null()) .stderr(Stdio::null()) @@ -36,7 +43,7 @@ pub fn open(store: &Store, task_id: i64) -> Result { std::thread::spawn(move || { let _ = child.wait(); }); - Ok(format!("opening {} in the browser", pr.url)) + Ok(format!("opening {url} in the browser")) } /// Pull a task's tracked PR's review comments into a reject-with-feedback body @@ -69,7 +76,8 @@ pub fn plan(store: &Store, task_id: i64) -> Result { /// state change — the task stays in `review` until a human accepts. The /// operator has already been gated by the caller; `pr` is operator-invoked, so /// the dispatched agent still cannot publish. Only called when no PR is -/// tracked; a tracked one jumps to [`open`] instead. +/// tracked; a tracked one jumps to [`open`] instead. Returns the canonical URL +/// so a caller can chain into [`open_url`] without re-reading the task. pub fn create(store: &mut Store, task_id: i64) -> Result { let plan = plan(store, task_id)?; let task = store.task(task_id).map_err(|e| e.to_string())?; @@ -84,7 +92,7 @@ pub fn create(store: &mut Store, task_id: i64) -> Result { store .set_pr(task_id, Some(&pr.url)) .map_err(|e| e.to_string())?; - Ok(format!("opened {} for task {task_id}", pr.url)) + Ok(pr.url) } /// Ask GitHub whether a review task's tracked PR still merges cleanly with its diff --git a/crates/voro/src/ui.rs b/crates/voro/src/ui.rs index 03e84a1..60df4b2 100644 --- a/crates/voro/src/ui.rs +++ b/crates/voro/src/ui.rs @@ -166,6 +166,7 @@ fn draw_mode(frame: &mut Frame, app: &App) { Span::raw("create a ready PR titled "), Span::styled(format!("“{title}”"), Style::new().fg(Color::Blue)), ]), + Line::from(Span::raw("and open it in the browser")), Line::default(), Line::from(Span::styled( "⏎/y confirm · esc/n cancel", @@ -2731,4 +2732,55 @@ mod tests { "header missing ready count: {header}" ); } + + /// The create-PR modal spells out every consequence of confirming, the + /// browser jump included (DESIGN.md §8), so the key's second half is not a + /// surprise. + #[test] + fn confirm_pr_modal_announces_the_browser_jump() { + use crate::app::{App, Mode}; + use ratatui::Terminal; + use ratatui::backend::TestBackend; + use voro_core::{NewTask, Store}; + + let mut store = Store::open_in_memory().unwrap(); + let p = store.create_project("voro", "/tmp/voro").unwrap(); + let task = store + .create_task(NewTask { + project_id: p.id, + repo_id: None, + title: "ship it".into(), + body: String::new(), + priority: Priority::P2, + state: TaskState::Ready, + agent: None, + human: false, + deep: false, + }) + .unwrap(); + + let ctx = crate::dispatch::DispatchCtx::from_db_path(std::path::Path::new( + "/nonexistent/voro.db", + )); + let mut app = App::new(store, ctx).unwrap(); + app.mode = Mode::ConfirmPr { + task_id: task.id, + branch: "feat/ship".into(), + title: "ship it".into(), + }; + + let mut terminal = Terminal::new(TestBackend::new(90, 24)).unwrap(); + terminal.draw(|f| draw(f, &app)).unwrap(); + let rendered = terminal + .backend() + .buffer() + .content() + .iter() + .map(|c| c.symbol()) + .collect::(); + assert!( + rendered.contains("open it in the browser"), + "modal should name the browser jump: {rendered}" + ); + } } diff --git a/docs/DESIGN.md b/docs/DESIGN.md index 095d091..e7d3363 100644 --- a/docs/DESIGN.md +++ b/docs/DESIGN.md @@ -289,7 +289,7 @@ Naming the id literally is what makes the return path survive the launch style t **Branch names** flow through dispatch in both directions, and Voro runs no git in either — it only passes a name in and records one back. A task carries an optional `branch` (schema §5): the *intended* name a human sets with `voro set --branch`, which is the mechanism for attaching a task to an existing branch as much as for naming a fresh one. When set, dispatch renders it into the prompt preamble — telling the agent to create or check out that branch itself before working, since the agent knows the checkout's state better than the dispatcher and Voro deliberately never touches the working tree. Either way — whether a human named the branch or the agent chooses its own — the preamble tells the agent to register that branch with `voro set --branch NAME` the moment it creates or checks it out, so Voro records the real branch while the task is still `running` (letting reconcile, attach, `voro pr`, and the UI reflect it, and capturing it even if the agent never reaches a clean `done`) rather than only learning it at completion. The reverse direction is the *reported* name: `voro done --branch NAME` (and, belt-and-braces, the `SessionEnd` hook in [`agent-integration.md`](agent-integration.md)) records the branch the work actually landed on, overwriting any intended name — and re-confirms the early-registered name for the assigned case. The intended name is a suggestion the agent may follow or override; the reported name is the source of truth. Storing it on the task rather than the session means it survives redispatch and reads naturally beside `pr_url`, so a task correlates with its PR and its branch at a glance; Voro never reads the checkout's HEAD to infer it, consistent with the task-state-versus-session-state boundary above. -**Opening the PR** is `pr`'s second job. On a `review` task with a tracked `pr_url` it is unchanged — jump to the PR in a browser (§11c). On one with *none* it *creates* the PR from the done-time state the two directions above capture: it asserts the task is in `review` and carries both a branch and a completion summary (erroring, network-free, on whichever is missing), pushes the branch to `origin`, opens a ready-for-review (non-draft) GitHub PR whose title is the task title and whose body is that summary, and records the URL through the same `set --pr` write path. No state change — the task stays `review` until a human accepts. The description is captured at `done` while the agent's context is hot, and the rest is mechanical. Crucially `pr` is operator-invoked, so Voro pushing on the operator's behalf preserves the trust model — the *dispatched agent* still cannot publish work (the one deliberate rule), the human running `pr` is the gate, and the PR page is where the diff gets reviewed. The CLI confirms interactively before pushing (`--yes` skips it); the TUI shows the same confirmation as a modal. The forge-specific half — push plus `gh pr create` — sits behind one seam in the `voro` crate, and that seam is where the two review media meet: `open` and `pr` are the same operation in two media — get the task's diff in front of the human, locally or on GitHub — so `pr` (one verb, one TUI key) *is* the per-project "show me this task's diff" action. Which medium it uses is the project's **review action** (`projects.review_action`, §5): `auto` (the unconfigured default) uses GitHub when the checkout is a GitHub repo — probed through `gh repo view`, with a missing or unauthenticated `gh` reading as "not GitHub" — and a `voro.toml` viewer otherwise; `pr` pins the GitHub flow, erroring on a checkout that cannot take one; `viewer[:name]` pins a viewer (§11a). On the viewer medium the task's diff is opened in that viewer exactly as `open` does — allowed on `review` and `running`, demanding no branch or summary and confirming nothing, since nothing is pushed. `open` survives as the explicit always-viewer spelling, for reaching the local diff even on a GitHub project. Because a dispatched agent works in a throwaway worktree on the task's branch (§11), the diff lives there, not in the primary checkout, so `open` runs the viewer in that worktree when the task's branch has a live one, falling back to the task's resolved repo (§3) when it has no branch or no worktree. The viewer template is filled with `{path}` (that resolved directory), `{branch}` (the task's branch, empty when none), and `{base}` (the checkout's default branch, read from `refs/remotes/origin/HEAD` with a `main` fallback) so it can express a diff range like `{base}...{branch}` rather than a bare directory; a template using none of these is substituted unchanged. The action is set per project with `voro project action` or the projects screen's picker (`v`), and viewers are defined as `[viewers.]` tables in `voro.toml` (§5) surfaced by `voro viewer list`. The medium decision itself (`ReviewAction::resolve`) plus the pure precondition check and plan assembly live in `voro-core` (tested); the seam supplies only the GitHub probe and the git/`gh` I/O, in the `voro` crate. +**Opening the PR** is `pr`'s second job. On a `review` task with a tracked `pr_url` it is unchanged — jump to the PR in a browser (§11c). On one with *none* it *creates* the PR from the done-time state the two directions above capture: it asserts the task is in `review` and carries both a branch and a completion summary (erroring, network-free, on whichever is missing), pushes the branch to `origin`, opens a ready-for-review (non-draft) GitHub PR whose title is the task title and whose body is that summary, and records the URL through the same `set --pr` write path. No state change — the task stays `review` until a human accepts. The description is captured at `done` while the agent's context is hot, and the rest is mechanical. Crucially `pr` is operator-invoked, so Voro pushing on the operator's behalf preserves the trust model — the *dispatched agent* still cannot publish work (the one deliberate rule), the human running `pr` is the gate, and the PR page is where the diff gets reviewed. The CLI confirms interactively before pushing (`--yes` skips it); the TUI shows the same confirmation as a modal, and on confirming it *also* jumps to the new PR in the browser, since creating one is all but always followed by looking at it and the operator would otherwise press the key twice. That chained open is cosmetic, not part of the create: the URL is recorded either way, so a browser that will not launch is reported beside the URL rather than as a failed create. The CLI leaves the chain to the operator, who is already at a shell. The forge-specific half — push plus `gh pr create` — sits behind one seam in the `voro` crate, and that seam is where the two review media meet: `open` and `pr` are the same operation in two media — get the task's diff in front of the human, locally or on GitHub — so `pr` (one verb, one TUI key) *is* the per-project "show me this task's diff" action. Which medium it uses is the project's **review action** (`projects.review_action`, §5): `auto` (the unconfigured default) uses GitHub when the checkout is a GitHub repo — probed through `gh repo view`, with a missing or unauthenticated `gh` reading as "not GitHub" — and a `voro.toml` viewer otherwise; `pr` pins the GitHub flow, erroring on a checkout that cannot take one; `viewer[:name]` pins a viewer (§11a). On the viewer medium the task's diff is opened in that viewer exactly as `open` does — allowed on `review` and `running`, demanding no branch or summary and confirming nothing, since nothing is pushed. `open` survives as the explicit always-viewer spelling, for reaching the local diff even on a GitHub project. Because a dispatched agent works in a throwaway worktree on the task's branch (§11), the diff lives there, not in the primary checkout, so `open` runs the viewer in that worktree when the task's branch has a live one, falling back to the task's resolved repo (§3) when it has no branch or no worktree. The viewer template is filled with `{path}` (that resolved directory), `{branch}` (the task's branch, empty when none), and `{base}` (the checkout's default branch, read from `refs/remotes/origin/HEAD` with a `main` fallback) so it can express a diff range like `{base}...{branch}` rather than a bare directory; a template using none of these is substituted unchanged. The action is set per project with `voro project action` or the projects screen's picker (`v`), and viewers are defined as `[viewers.]` tables in `voro.toml` (§5) surfaced by `voro viewer list`. The medium decision itself (`ReviewAction::resolve`) plus the pure precondition check and plan assembly live in `voro-core` (tested); the seam supplies only the GitHub probe and the git/`gh` I/O, in the `voro` crate. **Detecting a stale review branch.** A task can sit in `review` while other work merges, leaving its branch in conflict with the moved base. The operator usually learns this from the PR page; Voro surfaces it too, cheaply, for a `review` task with a tracked `pr_url`. GitHub already computes the answer: `gh pr view --json mergeable` returns `MERGEABLE`, `CONFLICTING`, or `UNKNOWN` (the last while GitHub recomputes — no signal, never a conflict). A `CONFLICTING` verdict is surfaced as an informational `[branch conflicts]` marker, read fresh and never stored — the same rendered-not-stored shape as the `[incomplete report]` flag. It only *tells* the operator the branch needs resolving before it can merge; Voro takes no action on it (there is no automatic rebase), and the next-action derivation is untouched (a review row's default verb stays `pr`). Because the probe is network I/O it must not run per rendered row the way that flag does, so it is on demand and single-task: `voro show ` and the TUI detail pane each probe only the one task in view, leaving the queue and `list` unannotated. `voro show` simply blocks on the call, which is what a one-shot CLI verb should do. The TUI cannot: a half-second `gh` round-trip on the render path freezes the event loop on every selection that lands on a review task, so the probe runs *off* the loop and *behind* the selection. Off the loop means a background thread runs the `gh` call and sends `(task id, verdict)` back over a channel the loop drains each tick; while a probe is in flight the pane shows nothing, which needs no pending state of its own because a missing signal is already never a conflict, and a verdict whose task is no longer selected is discarded rather than shown against the wrong row. Behind the selection means the probe starts only once the selection has *rested* on a row for a short settle interval (400ms), so scrolling the queue spawns nothing for the rows passed over and resting on a review task spawns exactly one probe — at most one in flight at a time. The verdict is held in memory against the selected id and dropped when the selection moves, so re-selecting a row still re-probes for a fresh answer. The "is a probe due" decision (selection, held verdict, in-flight probe, rest duration → start or not) is a pure function with tests; the thread and channel are the untested shell around it. `MERGEABLE`, `UNKNOWN`, and a missing or unauthenticated `gh` all show nothing — a missing signal is never a conflict. The `gh` shell-out lives in the `voro` crate beside the other seams; the verdict decision (`MERGEABLE`/`CONFLICTING`/`UNKNOWN` → marker or not) is pure and lives in `voro-core` with tests, the same task-state-versus-session-state split `pr` follows.