From 8e83e69caa8b431872280350fdcc0957f9679de3 Mon Sep 17 00:00:00 2001 From: Daniels-Main Date: Mon, 13 Jul 2026 21:55:41 +0200 Subject: [PATCH] Add inline PR comments and readiness ledger --- README.md | 12 +- ROADMAP.md | 26 ++ TASKS.md | 33 ++- crates/strand-tauri/src/commands.rs | 29 +++ crates/strand-tauri/src/main.rs | 1 + crates/strand-tauri/src/pull_requests.rs | 130 ++++++++++ docs/README.md | 2 + docs/pull-request-improvements.md | 313 +++++++++++++++++++++++ ui/src/components/Diff.tsx | 39 ++- ui/src/lib/pullRequests.test.ts | 84 ++++++ ui/src/lib/pullRequests.ts | 184 ++++++++++++- ui/src/lib/tauri.ts | 12 + ui/src/styles/features.css | 129 ++++++++++ ui/src/views/PullRequests.tsx | 264 +++++++++++++++++-- website/docs/pull-requests.md | 39 ++- 15 files changed, 1254 insertions(+), 43 deletions(-) create mode 100644 docs/pull-request-improvements.md diff --git a/README.md b/README.md index 6ea88fc..23c3614 100644 --- a/README.md +++ b/README.md @@ -65,12 +65,14 @@ keyboard alone, and the mouse stays first-class. opening automatically. Each PR gets a full-width workspace: read rendered Markdown descriptions and avatar-led timeline conversations, compose top-level comments with formatting, preview, and hosted screenshot links, inspect - lazily loaded code changes in the Local Changes-style Pierre file tree and - diff workspace, see color-coded provider checks, and merge with merge-commit, - squash, or rebase through a GitHub-style split merge control. + lazily loaded code changes in the Local Changes-style Pierre file tree, + switch stacked/split layout in place, and add stale-head-guarded GitHub + comments to selected line ranges. A compact readiness ledger combines review, + checks, conflicts, merge state, and provider freshness before merging with + merge-commit, squash, or rebase through a GitHub-style split control. Authentication stays in the signed-in `gh` / `az` CLI; provider policies - remain enforced, while submit-review and other lifecycle actions are still - in progress. + remain enforced. Azure inline comments need iteration tracking and + submit-review and other lifecycle actions are still in progress. - **Worktrees (⌘5)** — an AI-agent dashboard for every worktree with stable repo naming, branch/session labels, dirty count, ±lines, "touched 3m ago" activity, disk size, ahead/behind, and one-click Review pinned where the diff --git a/ROADMAP.md b/ROADMAP.md index 6172845..15846d6 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -1580,6 +1580,32 @@ common Markdown blocks, a hosted screenshot/image action, live character count, and clearer submission feedback. Remote images remain inert until the user explicitly reveals them, preserving the webview privacy boundary. +**Hosted PR review-workspace research (2026-07-13):** A current comparison of +Codex, Cursor, Conductor, GitHub, Graphite, and GitKraken is captured in +`docs/pull-request-improvements.md`. The recommended next phase keeps Strand's +lazy provider queries and one-mounted-diff boundary, but adds the missing review +state: a readiness/viewed/thread ledger, inline thread lifecycle and batched +review submission, attention-oriented list filters, then commit/version and +safe worktree actions. Concrete unchecked slices were added to `TASKS.md`; no +user-visible behavior changed in this research pass. + +**Hosted PR readiness ledger (2026-07-13):** The first review-workspace slice +landed in the PR header. `pullRequestReadiness` conservatively combines active / +draft state, exact-head availability, required-review state, CI results, merge +conflicts, and provider merge state; the compact `.pr-readiness` strip keeps the +result, checks summary, review decision, and update age visible across every PR +tab, with keyboard-accessible blocker details. Unrecognized signals and Azure's +currently unavailable policy/check detail remain **Status incomplete**, never a +false green. Focused Vitest coverage locks the provider-state normalization. + +**Hosted PR inline review kick (2026-07-13):** Changes now carries its own +persisted stacked/split controls and exposes Pierre's controlled line-range +selection through Strand's diff boundary. Open GitHub PRs get an annotation-row +composer that publishes a real review comment against old/new blob coordinates; +`repo_pull_request_inline_comment` rechecks the exact head before the write so a +stale diff cannot silently misplace feedback. Azure stays explicitly disabled +until its required iteration/change-tracking coordinates join the patch model. + --- ## 1.1+ — Post-1.0 diff --git a/TASKS.md b/TASKS.md index 70ba022..977a833 100644 --- a/TASKS.md +++ b/TASKS.md @@ -1284,7 +1284,8 @@ tree: watch the agent work, review fast, accept or reject safely. keyboard-operable 22% Pierre folder tree and compact Local Changes-style file header mount one selected, edge-to-edge diff at a time, leaving the rest of the full-width detail workspace for code and following the app's - split/stacked appearance settings. Azure comparisons + split/stacked appearance settings. The file header now exposes the same + persisted stacked/split controls in context. Azure comparisons fetch source/target objects without updating repository refs or FETCH_HEAD. - ◐ Discussion threads and comment creation: Conversation reads GitHub comments and Azure thread comments (including inline file context) as safe @@ -1293,10 +1294,38 @@ tree: watch the agent work, review fast, accept or reject safely. keyboard-operable Write/Preview composer, Markdown formatting toolbar, hosted screenshot/image insertion, explicit click-to-load image previews, character count, provider avatars with initials fallback, and comment - permalinks. Replies, new + permalinks. GitHub Changes now supports Pierre line-range selection and an + anchored inline composer through `repo_pull_request_inline_comment`, with + exact-head validation before publishing. Replies, Azure iteration-tracked inline comments, direct binary attachment uploads, suggestions, and thread resolution remain. - ☐ Submit reviews: approve, request changes, dismiss/update a review where supported. + - ◐ PR review ledger + merge-readiness model (see + `docs/pull-request-improvements.md`). + - ☑ Header readiness strip (`pullRequestReadiness`, `.pr-readiness`): + combines state, required reviews, checks, conflicts, provider freshness, + and expandable blocker evidence; missing Azure policy/check fields remain + explicitly incomplete instead of appearing ready. + - ☐ Add viewed-file progress and unresolved-thread counts when those review + state models land. + - ◐ Inline review workspace: GitHub line-range selection and immediate + publishing are present (`ParsedDiff` controlled selection + inline + annotation composer); provider-neutral line/file thread reading, Azure + iteration coordinates, replies, resolved/outdated state, local + content-hash-keyed viewed files, unviewed/thread filters, batched drafts, + and keyboard next-thread navigation remain while retaining one mounted + Pierre diff. + - ☐ Batched review submission: pending comments plus Comment / Approve / + Request changes, summary preview, exact-head stale guard, and draft + preservation when a provider write fails. + - ☐ PR inbox controls over the shallow list: Open / Needs review / Mine / + Draft / Closed filters, fuzzy search, attention-first sort, and row-level + review/check state without expanding provider queries per row. + - ☐ Lazy Commits + independent Checks tabs; keep readiness persistent in the + PR header and deep-link unsupported provider detail to the host. + - ☐ Review evolution + local action: reliable “since my last review” compare + where the provider exposes a boundary, safe Open in worktree / Update branch, + suggestions, and unresolved-feedback export for external agents. - ◐ Checks render provider states as green success, yellow running, red failure, or neutral. Azure policies, merge queue/auto-complete, and required-review detail remain. diff --git a/crates/strand-tauri/src/commands.rs b/crates/strand-tauri/src/commands.rs index 54bcaa9..ec19a2f 100644 --- a/crates/strand-tauri/src/commands.rs +++ b/crates/strand-tauri/src/commands.rs @@ -221,6 +221,35 @@ pub async fn repo_pull_request_comment(path: String, id: u64, body: String) -> C .await } +/// Add a provider review thread anchored to an exact file line range. +/// `expected_head` prevents a delayed editor from commenting on a newer diff. +#[tauri::command(async)] +pub async fn repo_pull_request_inline_comment( + path: String, + id: u64, + body: String, + file_path: String, + start_line: u32, + end_line: u32, + side: pull_requests::PullRequestDiffSide, + expected_head: String, +) -> CmdResult<()> { + run_blocking("pull request inline comment", move || { + pull_requests::add_inline_comment( + &path, + id, + &body, + &file_path, + start_line, + end_line, + side, + &expected_head, + ) + .map_err(|message| CmdError { message }) + }) + .await +} + /// Merge a hosted pull request through its provider. The expected source /// commit prevents merging unseen updates; provider policies remain enforced. #[tauri::command(async)] diff --git a/crates/strand-tauri/src/main.rs b/crates/strand-tauri/src/main.rs index 27ea9c3..26e1245 100644 --- a/crates/strand-tauri/src/main.rs +++ b/crates/strand-tauri/src/main.rs @@ -80,6 +80,7 @@ fn main() { commands::repo_pull_request, commands::repo_pull_request_diff, commands::repo_pull_request_comment, + commands::repo_pull_request_inline_comment, commands::repo_pull_request_merge, commands::repo_diff_unstaged, commands::repo_diff_staged, diff --git a/crates/strand-tauri/src/pull_requests.rs b/crates/strand-tauri/src/pull_requests.rs index 4976a02..960b7c1 100644 --- a/crates/strand-tauri/src/pull_requests.rs +++ b/crates/strand-tauri/src/pull_requests.rs @@ -112,6 +112,13 @@ pub struct PullRequestList { pub pull_requests: Vec, } +#[derive(Debug, Clone, Copy, Deserialize)] +#[serde(rename_all = "snake_case")] +pub enum PullRequestDiffSide { + Deletions, + Additions, +} + #[derive(Debug, Clone, PartialEq, Eq)] enum HostRepo { GitHub { @@ -174,6 +181,44 @@ pub fn add_comment(path: &str, id: u64, body: &str) -> Result<()> { } } +#[allow(clippy::too_many_arguments)] +pub fn add_inline_comment( + path: &str, + id: u64, + body: &str, + file_path: &str, + start_line: u32, + end_line: u32, + side: PullRequestDiffSide, + expected_head: &str, +) -> Result<()> { + validate_comment(body)?; + validate_commit(expected_head)?; + if file_path.trim().is_empty() || file_path.contains(['\r', '\n', '\0']) { + return Err("The inline comment file path is invalid".to_string()); + } + if start_line == 0 || end_line < start_line { + return Err("The inline comment line range is invalid".to_string()); + } + let (_, host) = host_for_path(path)?; + match host { + HostRepo::GitHub { owner, repo } => { + let current = detail_github(path, owner.clone(), repo.clone(), id)?; + if current.source_commit != expected_head { + return Err("The pull request changed while this comment was being written. Refresh Changes and select the lines again.".to_string()); + } + add_inline_comment_github( + path, owner, repo, id, body, file_path, start_line, end_line, side, + expected_head, + ) + } + HostRepo::Azure { .. } => Err( + "Inline Azure DevOps comments need iteration tracking metadata that Strand does not load yet. Open this pull request on Azure DevOps to comment on these lines." + .to_string(), + ), + } +} + pub fn merge( path: &str, id: u64, @@ -297,6 +342,61 @@ fn add_comment_github(cwd: &str, owner: String, repo: String, id: u64, body: &st Ok(()) } +#[allow(clippy::too_many_arguments)] +fn add_inline_comment_github( + cwd: &str, + owner: String, + repo: String, + id: u64, + body: &str, + file_path: &str, + start_line: u32, + end_line: u32, + side: PullRequestDiffSide, + expected_head: &str, +) -> Result<()> { + let endpoint = format!("repos/{owner}/{repo}/pulls/{id}/comments"); + let payload = github_inline_comment_payload( + body, file_path, start_line, end_line, side, expected_head, + ); + let input = serde_json::to_vec(&payload) + .map_err(|error| format!("Could not encode GitHub inline comment: {error}"))?; + run_command_input( + cwd, + "gh", + &["api", "--method", "POST", &endpoint, "--input", "-"], + &[("GH_PROMPT_DISABLED", "1")], + Some(&input), + )?; + Ok(()) +} + +fn github_inline_comment_payload( + body: &str, + file_path: &str, + start_line: u32, + end_line: u32, + side: PullRequestDiffSide, + expected_head: &str, +) -> Value { + let side = match side { + PullRequestDiffSide::Deletions => "LEFT", + PullRequestDiffSide::Additions => "RIGHT", + }; + let mut payload = serde_json::json!({ + "body": body, + "commit_id": expected_head, + "path": file_path, + "line": end_line, + "side": side, + }); + if start_line != end_line { + payload["start_line"] = start_line.into(); + payload["start_side"] = side.into(); + } + payload +} + fn merge_github( cwd: &str, owner: &str, @@ -1293,4 +1393,34 @@ mod tests { assert_eq!(azure_merge_strategy(PullRequestMergeStrategy::MergeCommit), "noFastForward"); assert_eq!(azure_merge_strategy(PullRequestMergeStrategy::Rebase), "rebase"); } + + #[test] + fn builds_github_inline_comment_ranges_with_blob_sides() { + let multi = github_inline_comment_payload( + "Please simplify this.", + "src/lib.rs", + 10, + 12, + PullRequestDiffSide::Additions, + "0123456789abcdef0123456789abcdef01234567", + ); + assert_eq!(multi["path"], "src/lib.rs"); + assert_eq!(multi["start_line"], 10); + assert_eq!(multi["line"], 12); + assert_eq!(multi["start_side"], "RIGHT"); + assert_eq!(multi["side"], "RIGHT"); + + let single = github_inline_comment_payload( + "Why remove this?", + "src/lib.rs", + 7, + 7, + PullRequestDiffSide::Deletions, + "0123456789abcdef0123456789abcdef01234567", + ); + assert_eq!(single["line"], 7); + assert_eq!(single["side"], "LEFT"); + assert!(single.get("start_line").is_none()); + assert!(single.get("start_side").is_none()); + } } diff --git a/docs/README.md b/docs/README.md index c2f4f36..dde236b 100644 --- a/docs/README.md +++ b/docs/README.md @@ -5,6 +5,8 @@ Project documentation. - [`learnings.md`](./learnings.md) — durable rules learned while building Strand. Read before touching the UI; append when you discover something future work has to respect. +- [`pull-request-improvements.md`](./pull-request-improvements.md) — competitive + research and the prioritized UX/UI proposal for the hosted PR workspace. - [`changelog/`](./changelog) — append-only log of substantive changes per working session. One file per session, dated. - See also: [`../ROADMAP.md`](../ROADMAP.md) (milestones) and diff --git a/docs/pull-request-improvements.md b/docs/pull-request-improvements.md new file mode 100644 index 0000000..3e9b577 --- /dev/null +++ b/docs/pull-request-improvements.md @@ -0,0 +1,313 @@ +# Pull-request workspace: competitive review and improvement proposal + +> Research date: 2026-07-13. This is a product/UX proposal, not an implementation +> record. It compares current documented behavior with Strand's existing +> `PullRequests.tsx` workspace and keeps Strand's performance, keyboard, provider- +> neutral, and resizable-pane rules as hard constraints. + +> Implementation status (2026-07-13): the readiness ledger, in-context +> stacked/split controls, Pierre line-range selection, and stale-head-guarded +> GitHub inline publishing are now present on `codex/pr-review-ledger`. Inline +> comments publish immediately in this first slice; the pending **Add to +> review** queue described below remains the intended batched-review workflow. + +## Outcome + +Strand already has a good *PR reader*: a fast list, lazy detail and patch +queries, provider-neutral GitHub/Azure support, safe Markdown, comments, a +single-mounted-file diff, and guarded merge actions. The next step should not be +more metadata. It should turn the view into a *review workspace* that keeps a +reviewer's place and makes the next blocker or action obvious. + +The proposed signature is a **review ledger**: one compact, persistent status +line shared by the PR header and changed-file tree: + +`14 / 22 viewed · 3 unresolved · 1 failing check · updated 8m ago` + +Each file carries the same small state vocabulary (unviewed, viewed, changed +since viewed, unresolved thread). This is specific to Strand's job—reviewing +large human- and agent-authored changes without losing one's place—and is more +useful than adding a generic dashboard or decorative status cards. + +## What comparable products do + +| Product | Integration model | Useful interaction pattern | Lesson for Strand | +| --- | --- | --- | --- | +| **Codex app** | When the checked-out branch has a GitHub PR, the sidebar loads PR context and reviewer feedback through authenticated `gh`; comments appear alongside the diff. The user can ask Codex to address selected feedback, inspect the resulting local diff, then stage, commit, and push in the same task. | Review feedback is not a destination; it becomes actionable context for a fix-and-verify loop. | Keep hosted threads attached to their code, and offer a lightweight handoff such as **Copy unresolved feedback** before considering a built-in agent. | +| **Cursor 3** | A PR workspace spans Reviews, Commits, and Changes. Reviews combines inline threads and top-level comments; Changes uses a file tree and changes picker. Reviewer status, pending-review banners, and quick-action pills stay prominent. | The interface surfaces the current review state and likely next action instead of making the user infer them from raw metadata. | Add a commits view, thread-aware review state, and contextual actions; do not turn every action into a permanent toolbar button. | +| **Conductor** | The PR is attached to the same worktree/workspace as the agent, terminal, diff, and branch. Its Checks tab aggregates git status, PR metadata, CI, deployments, review comments, and todos. Line comments can be sent back to the agent; resolved GitHub threads can be cleared in the diff. | “Ready to merge?” is a cross-signal question, not merely a CI state. | Build one provider-neutral readiness model and let it drive both the summary and merge affordance. | +| **GitHub** | Conversation, Commits, Checks, and Files changed separate distinct review jobs. Files changed supports single- or multi-line comments, file comments, suggestions, batched reviews, and a navigator for unresolved, resolved, and outdated conversations. | Threads have lifecycle and location; reviews have draft state and a deliberate submit step. | Replies, resolution, outdated state, and review submission are core data-model work, not conversation-tab polish. | +| **Graphite** | A filtered PR inbox groups work into needs-review, returned, approved, drafts, and merging states. The review page supports inline threads/suggestions, keyboard review actions, version comparison, and “hide reviewed changes.” | Reviewers need an attention queue and a way to isolate what changed since their last pass. | First add local viewed-state and list filters; later add provider-backed “since last review” comparison if the provider exposes a reliable boundary. | +| **GitKraken** | Desktop exposes PR metadata, checkout, build status, suggestions, and merge, but its newest full review experience is linked to a richer web surface. | A desktop client can be excellent without reproducing every host feature, provided deep links preserve context. | Keep **Open on host** and add targeted links for a check or thread when provider support is incomplete. | + +Sources: + +- [Codex code review and pull-request workflow](https://learn.chatgpt.com/docs/code-review.md) +- [Codex code review in GitHub](https://learn.chatgpt.com/docs/third-party/github.md) +- [Cursor 3 PR review announcement](https://cursor.com/changelog/05-07-26) +- [Conductor: review and merge a workspace](https://www.conductor.build/docs/guides/review-and-merge) +- [Conductor Checks reference](https://www.conductor.build/docs/reference/checks) +- [GitHub: commenting on a pull request](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/commenting-on-a-pull-request) +- [GitHub: about pull requests](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/about-pull-requests) +- [Graphite PR page](https://graphite.com/docs/pr-page-overview) +- [Graphite PR inbox](https://graphite.com/docs/use-pr-inbox) +- [Graphite PR versions](https://graphite.com/docs/pull-request-versions) +- [GitKraken pull requests](https://help.gitkraken.com/gitkraken-desktop/pull-requests/) + +## Strand today + +### What is already strong + +- The list query is intentionally shallow and detail loads only after a PR is + opened. That avoids the provider query-cap failure already recorded in + `ROADMAP.md`. +- Changes load only when requested, parse once, and mount one Pierre diff at a + time. This is the correct performance boundary for large PRs. +- List, tabs, file tree, merge menu, composer, and command-palette entry points + already have credible keyboard models. +- Provider Markdown is sanitized and remote images require an explicit reveal. +- Merge carries the displayed head SHA, so a newly-pushed commit cannot be + merged without review. +- The list-to-full-width-detail navigation gives the diff enough room instead + of permanently shrinking it beside a PR list. + +### Friction in the current information architecture + +1. **The list is an archive, not an inbox.** Open, closed, and merged PRs share + one latest-100 sequence with no search, state filter, “needs my review,” or + “authored by me” grouping. +2. **Status is descriptive rather than diagnostic.** Raw review and merge-state + pills do not explain whether the blocker is a failed check, missing approval, + draft state, conflict, unresolved thread, or unknown provider data. +3. **Overview buries readiness.** Reviewers and checks appear after labels and + description, even though they determine the next action. +4. **Conversation flattens review structure.** Top-level comments are readable, + and Azure paths are shown, but replies, inline placement, resolved/outdated + state, and pending review comments are absent. +5. **Changes does not preserve review progress.** There is no viewed state, + changed-since-viewed state, unresolved-thread count, or next-thread command. +6. **No review decision can be completed in Strand.** Approve, request changes, + review summary, and batched submission are still host-only. +7. **No commit/version lens.** A reviewer cannot understand how the PR evolved + or isolate changes since an earlier pass. +8. **Refresh is manual.** CI and review feedback can become stale while the PR + remains open, with no visible “last updated” age. + +## Recommended UX and UI + +### P0 — complete the review loop + +#### 1. Add the review ledger and readiness summary + +Replace the free-form status pills with a compact, semantic strip directly +under the title. It should report only facts Strand can support: + +- **Ready to merge** when all provider-reported required signals are satisfied. +- **3 blockers** with an expandable ordered list: draft, conflicts, required + reviews, failed/running required checks, unresolved required threads, stale + branch, then provider-specific policy. +- **Status incomplete** when Azure or another provider does not expose a signal; + unknown must never be rendered as success. +- Review progress: viewed files and unresolved threads. +- Freshness: “updated 2m ago” with a refresh action when stale. + +Keep the merge control visible, but make its relationship to readiness explicit. +If the provider permits an override, the confirmation should name the blockers; +if Strand cannot determine readiness, say “Provider will verify on merge.” + +#### 2. Make Changes the primary review workspace + +Keep the current two-pane, single-mounted-file layout. Enrich it rather than +adding an always-visible third pane: + +- File rows gain a small viewed toggle, unresolved-thread count, and + changed-since-viewed indicator. Key viewed state by PR head SHA + file patch + hash so a new push safely reopens changed files. +- The file-tree header becomes the ledger: `14 / 22 viewed · 3 unresolved`, plus + filters for **All**, **Unviewed**, and **Threads**. +- `]` / `[` move to next/previous file; `n` / `Shift+n` move between unresolved + threads; `v` toggles viewed. Expose all through the command palette and show + the shortcuts in tooltips. +- Hovering or focusing a diff line reveals **Add comment**. Shift-selection + creates a multi-line range. Existing threads render immediately after their + anchor line and can collapse without unmounting the file diff. +- A compact conversation navigator popover lists unresolved, resolved, and + outdated threads. Selecting an item switches file and scrolls to its anchor. + +This is the visual signature: the changed-file tree becomes a quiet ledger of +what the reviewer has and has not cleared. Use existing `--text-*`, `--accent`, +`--warn`, `--add`, and `--del` tokens; do not introduce another status palette. + +#### 3. Add a real review draft and submission flow + +Line/file comments should default to **Add to review**, not notify immediately. +A sticky **Finish review (3)** action opens a focused sheet with: + +- the pending comments grouped by file; +- an optional Markdown summary; +- **Comment**, **Approve**, and **Request changes** as mutually exclusive + outcomes, limited by provider capability; +- the exact head SHA being reviewed and a stale-head refusal before submission. + +After submission, keep the user on the same file and announce the result with an +ARIA live region. Replies and Resolve/Reopen belong inline in the thread. When a +provider cannot support a write, preserve the draft and offer a direct host link. + +### P1 — make attention and evolution visible + +#### 4. Turn the list into a repository-level PR inbox + +The first iteration should stay local and cheap—filter the existing shallow +result rather than multiplying provider calls: + +- segmented filters: **Open**, **Needs review**, **Mine**, **Draft**, **Closed**; +- fuzzy search over number, title, author, and branch; +- row-leading attention state: review requested, changes requested, failing CI, + draft, or ready; +- updated age and comment/check summary at the trailing edge; +- sort open work by action required, then recency; keep closed/merged behind + their filter rather than mixing them into the default view. + +Do not copy Graphite's cross-repository inbox into this surface. Strand already +has a workspace-level aggregation pattern; cross-repo PR triage should be a +separate later decision. + +#### 5. Split Summary, Commits, and Checks by job + +Rename **Overview** to **Summary** and place description first, followed by a +short metadata block. Add: + +- **Commits**: chronological commit rows with author, subject, time, and a + compare/open action. Fetch only when the tab opens. +- **Checks**: grouped required, failed, running, and successful checks with + duration and provider deep links. Fetch/refresh independently so a CI update + does not re-download comments or the patch. + +The persistent readiness strip means reviewers never need to visit Checks just +to learn whether merging is blocked. + +#### 6. Add branch/worktree actions in context + +For a PR not currently checked out, show **Open in worktree…** as the preferred +action and **Check out branch** in its menu. For the current branch, show +**Update branch** only when the provider reports it behind the base. These should +reuse Strand's existing worktree safety and overlap checks; a hosted PR must not +quietly take over a dirty checkout. + +#### 7. Refresh by signal, not by whole-page polling + +- Show the last successful provider refresh time. +- Refresh lightweight readiness data on window focus and on a modest interval + only while the PR view is visible. +- Keep detail, threads, checks, commits, and patch caches separate. +- Invalidate the patch and file-viewed hashes only when the head SHA changes. +- Never poll the patch or mount additional Pierre diffs in the background. + +### P2 — reduce repeat-review cost + +#### 8. Add “changes since my last review” + +When the provider exposes the viewer's last submitted review commit, add a +compare selector: + +- **All changes**: merge base → current head; +- **Since my review**: last reviewed head → current head; +- **Commit…**: selected PR commit → current head. + +If no reliable review boundary exists, omit the mode rather than infer it from +timestamps. Viewed-state remains local and clearly labeled as such. + +#### 9. Add suggestions and a feedback handoff + +- Let a line-range comment switch to **Suggest change**, producing provider- + native suggestion syntax where supported. +- Add **Copy unresolved feedback** to export thread links, paths, ranges, and + bodies as compact Markdown suitable for an external coding agent. +- If Strand later gains a first-class agent handoff, attach the exact PR head + SHA and selected threads; do not begin with a vague “Fix PR” button. + +#### 10. Support provider lifecycle features without polluting the common UI + +Close/reopen, ready-for-review, reviewer management, auto-merge, merge queues, +and deployments should be capability-gated actions. Put uncommon provider- +specific operations in an overflow menu or readiness detail, not beside the +primary review/merge actions. + +## Proposed layout + +```text +┌ Pull Requests / #482 Refresh ··· ┐ +│ Fix stale cache invalidation Open on host │ +│ #482 · daniel · feature/cache → main │ +│ [2 blockers] 14/22 viewed · 3 unresolved · CI 8/9 · updated 2m ago │ +├ Summary Conversation Changes Commits Checks ┤ +│ │ +│ ┌ Changed files ───────────────┐┆┌ src/cache.ts ─ +42 −18 ────────┐ │ +│ │ 14 / 22 viewed · 3 threads │┆│ 81 const ttl = ... │ │ +│ │ [All] [Unviewed] [Threads] │┆│ +82 cache.set(key, value) [+] │ │ +│ │ ○ src/api.ts 1 │┆│ ┌ unresolved · maya · 8m ────┐ │ │ +│ │ ◐ src/cache.ts 2 │┆│ │ This races the eviction… │ │ │ +│ │ ✓ src/types.ts │┆│ │ Reply Resolve Open host │ │ │ +│ │ ○ tests/cache.test.ts │┆│ └────────────────────────────┘ │ │ +│ └───────────────────────────────┘┆└─────────────────────────────────┘ │ +├ [Previous thread] [Next thread] [Finish review (3)] ┤ +``` + +The divider remains `react-resizable-panels` with a stable `autoSaveId`. At +narrow desktop widths, collapse the file tree to a files popover; do not squeeze +the diff below the project's 30% minimum. Inline threads are part of the diff +flow, not a fixed-width third rail. + +## Interaction and accessibility contract + +- Preserve listbox semantics for the PR inbox and tab semantics for detail. +- Focus returns to the originating row, line, or thread after sheets/popovers + close. New provider data must not steal focus. +- Every new action is in the command palette. Dedicated shortcuts are optional; + when present, use the platform modifier helper rather than hardcoded Cmd/Ctrl. +- Thread anchors remain reachable after collapse and expose path + line range to + screen readers. +- Status never relies on color alone: icon, label, and count travel together. +- Reduced motion means no animated progress shimmer or automatic scroll beyond + the explicitly selected thread/file. + +## Performance and data contract + +The implementation should keep separate lazy resources: + +```text +PR list (shallow) + └─ selected PR summary/readiness + ├─ conversation + review threads + ├─ checks + ├─ commits + └─ patch → parsed file metadata → one mounted file diff +``` + +- Do not expand threads, checks, and commits across the 100-row list query. +- Cache each resource by provider/repository/PR/head SHA and cancel stale + requests when selection changes. +- Fetch small readiness deltas separately from the patch. +- Keep comments paginated or incrementally loaded for very active PRs. +- Store viewed state as `{pr, headSha, path, patchHash}`; a changed hash becomes + unviewed automatically. +- Thread anchors should use provider line/start-line/side fields, not deprecated + diff-relative positions. Preserve enough original metadata to deep-link when + a thread becomes outdated. +- Measure list-to-interactive, PR-open-to-summary, Changes-open-to-first-diff, + and thread-jump latency against PRD §8 before shipping. + +## Suggested delivery slices + +1. **Review foundation:** provider-neutral thread/review types, readiness model, + and the header ledger. Verify unknown provider fields never become green. +2. **Review workspace:** inline thread reading, thread navigator, and local + viewed-state/filtering. Verify one Pierre file remains mounted. +3. **Review writes:** pending comments, replies, resolve/reopen, and submit + comment/approve/request-changes with stale-head protection. +4. **Attention:** list filters/search/row states and independent Checks tab. +5. **Evolution:** Commits and “since my review” comparison where reliable. +6. **Local action:** safe worktree checkout/update and feedback export. + +Each slice should land provider capability tests, keyboard tests, and one large- +PR performance check with the feature—not as a cleanup pass afterward. diff --git a/ui/src/components/Diff.tsx b/ui/src/components/Diff.tsx index d7bae9a..144c8d2 100644 --- a/ui/src/components/Diff.tsx +++ b/ui/src/components/Diff.tsx @@ -1,6 +1,11 @@ -import { getSingularPatch, type FileDiffMetadata } from '@pierre/diffs'; +import { + getSingularPatch, + type DiffLineAnnotation, + type FileDiffMetadata, + type SelectedLineRange, +} from '@pierre/diffs'; import { FileDiff as PierreFileDiff, PatchDiff } from '@pierre/diffs/react'; -import { useMemo, type CSSProperties } from 'react'; +import { useMemo, type CSSProperties, type ReactNode } from 'react'; import { hashPatch } from '../lib/patch'; import { useSettings, type SettingsState } from '../stores/settings'; @@ -24,8 +29,15 @@ export interface DiffProps { style?: CSSProperties; } -export interface ParsedDiffProps extends Omit { +export interface ParsedDiffProps extends Omit { fileDiff: FileDiffMetadata; + /** Controlled line selection used by hosted review surfaces. */ + selectedLines?: SelectedLineRange | null; + /** Inline rows anchored to a side + line in the parsed diff. */ + lineAnnotations?: DiffLineAnnotation[]; + renderAnnotation?: (annotation: DiffLineAnnotation) => ReactNode; + /** Pierre emits a complete range after pointer or keyboard selection. */ + onLineSelected?: (range: SelectedLineRange | null) => void; } /** @@ -119,13 +131,17 @@ export function Diff({ } /** Render an already-parsed provider patch through Strand's Pierre boundary. */ -export function ParsedDiff({ +export function ParsedDiff({ fileDiff, layout = 'unified', hideFileHeader = false, + selectedLines, + lineAnnotations, + renderAnnotation, + onLineSelected, className, style, -}: ParsedDiffProps) { +}: ParsedDiffProps) { const pierreTheme = useSettings((s) => s.resolvedTheme) === 'light' ? 'pierre-light' : 'pierre-dark'; const diffIndicators = useSettings((s) => s.diffIndicators); const diffLineNumbers = useSettings((s) => s.diffLineNumbers); @@ -135,9 +151,20 @@ export function ParsedDiff({ theme: pierreTheme, disableBackground: true, disableFileHeader: hideFileHeader, + enableLineSelection: Boolean(onLineSelected), + controlledSelection: Boolean(onLineSelected), + onLineSelected, ...diffAppearanceOptions({ diffIndicators, diffLineNumbers, diffWordHighlight }), } as const; return ( - + + fileDiff={fileDiff} + options={options} + selectedLines={selectedLines} + lineAnnotations={lineAnnotations} + renderAnnotation={renderAnnotation} + className={className} + style={style} + /> ); } diff --git a/ui/src/lib/pullRequests.test.ts b/ui/src/lib/pullRequests.test.ts index 676bd84..353cffb 100644 --- a/ui/src/lib/pullRequests.test.ts +++ b/ui/src/lib/pullRequests.test.ts @@ -17,9 +17,42 @@ const { diffStats, markdownUrl, parsePullRequestPatch, + pullRequestReadiness, pullRequestForBranch, + relativeTimeLabel, } = await import('./pullRequests'); +import type { PullRequest } from './types'; + +function pullRequest(overrides: Partial = {}): PullRequest { + return { + id: 42, + title: 'Ship it', + state: 'open', + is_draft: false, + author: 'octo', + source_branch: 'feature', + source_commit: '1'.repeat(40), + target_branch: 'main', + created_at: '2026-07-13T10:00:00Z', + updated_at: '2026-07-13T11:00:00Z', + url: 'https://github.com/acme/repo/pull/42', + description: '', + merge_status: 'CLEAN', + review_status: 'APPROVED', + comment_count: 0, + commit_count: 1, + additions: 10, + deletions: 2, + changed_files: 2, + labels: [], + reviewers: [], + checks: [{ name: 'CI', status: 'SUCCESS' }], + comments: [], + ...overrides, + }; +} + describe('checkTone', () => { it('normalizes provider success, running, and failure states', () => { expect(checkTone('SUCCESS')).toBe('success'); @@ -30,6 +63,57 @@ describe('checkTone', () => { }); }); +describe('pullRequestReadiness', () => { + it('reports a GitHub PR ready only when every reported signal is clear', () => { + const readiness = pullRequestReadiness(pullRequest(), 'git_hub'); + expect(readiness.tone).toBe('ready'); + expect(readiness.label).toBe('Ready to merge'); + expect(readiness.checks).toMatchObject({ passed: 1, failed: 0, total: 1 }); + }); + + it('collects explicit blockers and pending signals', () => { + const readiness = pullRequestReadiness(pullRequest({ + merge_status: 'DIRTY', + review_status: 'CHANGES_REQUESTED', + checks: [ + { name: 'CI', status: 'FAILURE' }, + { name: 'Browser', status: 'IN_PROGRESS' }, + ], + }), 'git_hub'); + expect(readiness.tone).toBe('blocked'); + expect(readiness.label).toBe('3 blockers'); + expect(readiness.details).toContain('The source branch has merge conflicts.'); + expect(readiness.details).toContain('1 check is still running.'); + }); + + it('keeps incomplete Azure policy data neutral instead of ready', () => { + const readiness = pullRequestReadiness(pullRequest({ + merge_status: 'succeeded', + checks: [], + }), 'azure_dev_ops'); + expect(readiness.tone).toBe('neutral'); + expect(readiness.label).toBe('Status incomplete'); + expect(readiness.details[0]).toContain('Azure policy'); + }); + + it('treats a requested required reviewer as pending', () => { + const readiness = pullRequestReadiness(pullRequest({ + review_status: 'REVIEW_REQUIRED', + reviewers: [{ name: 'Ada', status: 'requested', required: true }], + }), 'git_hub'); + expect(readiness.tone).toBe('pending'); + expect(readiness.details).toEqual(['A required review is still pending.']); + }); +}); + +describe('relativeTimeLabel', () => { + it('formats provider freshness without baking time into the component', () => { + expect(relativeTimeLabel('2026-07-13T11:52:00Z', Date.parse('2026-07-13T12:00:00Z'))) + .toBe('8m ago'); + expect(relativeTimeLabel('')).toBe('Update time unavailable'); + }); +}); + describe('parsePullRequestPatch', () => { it('returns provider patch files and line totals', () => { const [file] = parsePullRequestPatch(`diff --git a/a.txt b/a.txt diff --git a/ui/src/lib/pullRequests.ts b/ui/src/lib/pullRequests.ts index 6133e65..28ff05b 100644 --- a/ui/src/lib/pullRequests.ts +++ b/ui/src/lib/pullRequests.ts @@ -1,10 +1,27 @@ import { parsePatchFiles, type FileDiffMetadata } from '@pierre/diffs'; import { hashPatch } from './patch'; -import type { PullRequest } from './types'; +import type { PullRequest, PullRequestProvider } from './types'; export type CheckTone = 'success' | 'running' | 'failed' | 'neutral'; +export type PullRequestReadinessTone = 'ready' | 'blocked' | 'pending' | 'neutral'; + +export interface PullRequestReadiness { + tone: PullRequestReadinessTone; + label: string; + summary: string; + details: string[]; + checks: { + passed: number; + running: number; + failed: number; + unknown: number; + total: number; + }; + reviewLabel: string; +} + export function checkTone(status: string): CheckTone { const normalized = status.trim().toUpperCase().replaceAll('-', '_').replaceAll(' ', '_'); if (['SUCCESS', 'SUCCEEDED', 'SUCCESSFUL', 'PASSED', 'PASS'].includes(normalized)) { @@ -19,6 +36,171 @@ export function checkTone(status: string): CheckTone { return 'neutral'; } +function normalizedStatus(status: string): string { + return status.trim().toUpperCase().replaceAll('-', '_').replaceAll(' ', '_'); +} + +function readableStatus(status: string): string { + const value = status.trim().replaceAll('_', ' ').toLowerCase(); + return value ? value[0].toUpperCase() + value.slice(1) : ''; +} + +/** + * Summarize only readiness signals the provider actually returned. In + * particular, Azure does not expose its complete policy/check state through + * the current CLI query, so absent policy data must never become “ready”. + */ +export function pullRequestReadiness( + pr: PullRequest, + provider: PullRequestProvider, +): PullRequestReadiness { + const checks = { passed: 0, running: 0, failed: 0, unknown: 0, total: pr.checks.length }; + for (const check of pr.checks) { + const tone = checkTone(check.status); + if (tone === 'success') checks.passed += 1; + else if (tone === 'running') checks.running += 1; + else if (tone === 'failed') checks.failed += 1; + else checks.unknown += 1; + } + + const details: string[] = []; + const pending: string[] = []; + const unknown: string[] = []; + const state = normalizedStatus(pr.state); + const review = normalizedStatus(pr.review_status); + const merge = normalizedStatus(pr.merge_status); + const active = state === 'OPEN' || state === 'ACTIVE'; + + if (!active) { + const summary = state === 'COMPLETED' || state === 'MERGED' + ? 'Merged pull request' + : state === 'ABANDONED' || state === 'CLOSED' + ? 'Closed pull request' + : 'Inactive pull request'; + return { + tone: 'neutral', + label: 'Read-only history', + summary, + details, + checks, + reviewLabel: readableStatus(pr.review_status) || 'No review status', + }; + } + + if (pr.is_draft) pending.push('Mark the pull request ready for review.'); + if (!pr.source_commit) details.push('Refresh to load the current source commit before merging.'); + + if (checks.failed > 0) { + details.push(`${checks.failed} ${checks.failed === 1 ? 'check is' : 'checks are'} failing.`); + } + if (checks.running > 0) { + pending.push(`${checks.running} ${checks.running === 1 ? 'check is' : 'checks are'} still running.`); + } + if (checks.unknown > 0) { + unknown.push(`${checks.unknown} ${checks.unknown === 1 ? 'check has' : 'checks have'} an unrecognized status.`); + } + + if (review === 'CHANGES_REQUESTED' || review === 'REJECTED') { + details.push('A reviewer requested changes.'); + } else if (review === 'REVIEW_REQUIRED' || review === 'REQUIRED') { + pending.push('A required review is still pending.'); + } else if (review && review !== 'APPROVED') { + unknown.push(`Review state “${readableStatus(pr.review_status)}” is not recognized.`); + } + + const requiredReviewers = pr.reviewers.filter((reviewer) => reviewer.required); + if (requiredReviewers.some((reviewer) => { + const status = normalizedStatus(reviewer.status); + return status === 'REJECTED' || status === 'CHANGES_REQUESTED' || status === 'WAITING_FOR_AUTHOR'; + })) { + if (!details.includes('A reviewer requested changes.')) { + details.push('A required reviewer requested changes.'); + } + } else if (requiredReviewers.some((reviewer) => { + const status = normalizedStatus(reviewer.status); + return status === 'REQUESTED' || status === 'NO_VOTE' || status === 'PENDING'; + })) { + if (!pending.includes('A required review is still pending.')) { + pending.push('A required review is still pending.'); + } + } + + if (['DIRTY', 'CONFLICTS', 'CONFLICT'].includes(merge)) { + details.push('The source branch has merge conflicts.'); + } else if (['BLOCKED', 'REJECTED_BY_POLICY', 'FAILURE', 'FAILED'].includes(merge)) { + if (details.length === 0) details.push('The provider reports that merging is blocked.'); + } else if (merge === 'BEHIND') { + pending.push('The source branch is behind the target branch.'); + } else if (merge === 'UNSTABLE') { + pending.push('The provider reports an unstable merge state.'); + } else if (merge === 'QUEUED') { + pending.push('The provider is still evaluating merge readiness.'); + } else if (!['CLEAN', 'SUCCEEDED', 'HAS_HOOKS'].includes(merge)) { + unknown.push(merge + ? `Merge state “${readableStatus(pr.merge_status)}” is not recognized.` + : 'Merge readiness was not reported.'); + } + + if (provider === 'azure_dev_ops') { + unknown.push('Azure policy and check details are not reported by this integration.'); + } + + if (details.length > 0) { + return { + tone: 'blocked', + label: `${details.length} ${details.length === 1 ? 'blocker' : 'blockers'}`, + summary: 'Resolve before merging', + details: [...details, ...pending, ...unknown], + checks, + reviewLabel: readableStatus(pr.review_status) || 'No review status', + }; + } + if (pending.length > 0) { + return { + tone: 'pending', + label: pr.is_draft ? 'Draft pull request' : `${pending.length} ${pending.length === 1 ? 'item' : 'items'} pending`, + summary: 'Not ready to merge yet', + details: [...pending, ...unknown], + checks, + reviewLabel: readableStatus(pr.review_status) || 'No review status', + }; + } + if (unknown.length > 0) { + return { + tone: 'neutral', + label: 'Status incomplete', + summary: 'Provider will verify on merge', + details: unknown, + checks, + reviewLabel: readableStatus(pr.review_status) || 'No review status', + }; + } + return { + tone: 'ready', + label: 'Ready to merge', + summary: 'No blockers reported', + details, + checks, + reviewLabel: readableStatus(pr.review_status) || 'No review required', + }; +} + +export function relativeTimeLabel(value: string, now = Date.now()): string { + const date = new Date(value); + if (!value || Number.isNaN(date.getTime())) return 'Update time unavailable'; + const seconds = Math.round((date.getTime() - now) / 1_000); + const absolute = Math.abs(seconds); + if (absolute < 5) return 'just now'; + const compact = absolute < 60 + ? `${absolute}s` + : absolute < 3_600 + ? `${Math.round(absolute / 60)}m` + : absolute < 86_400 + ? `${Math.round(absolute / 3_600)}h` + : `${Math.round(absolute / 86_400)}d`; + return seconds < 0 ? `${compact} ago` : `in ${compact}`; +} + export function parsePullRequestPatch(patch: string): FileDiffMetadata[] { if (!patch.trim()) return []; return parsePatchFiles(patch, `pr:${hashPatch(patch)}`, true).flatMap((parsed) => parsed.files); diff --git a/ui/src/lib/tauri.ts b/ui/src/lib/tauri.ts index aa1bcd5..72f3a33 100644 --- a/ui/src/lib/tauri.ts +++ b/ui/src/lib/tauri.ts @@ -104,6 +104,18 @@ export const tauri = { invoke('repo_pull_request_diff', { path, id }), repoPullRequestComment: (path: string, id: number, body: string) => invoke('repo_pull_request_comment', { path, id, body }), + repoPullRequestInlineComment: ( + path: string, + id: number, + body: string, + filePath: string, + startLine: number, + endLine: number, + side: 'deletions' | 'additions', + expectedHead: string, + ) => invoke('repo_pull_request_inline_comment', { + path, id, body, filePath, startLine, endLine, side, expectedHead, + }), repoPullRequestMerge: ( path: string, id: number, diff --git a/ui/src/styles/features.css b/ui/src/styles/features.css index a113b87..f1bbb26 100644 --- a/ui/src/styles/features.css +++ b/ui/src/styles/features.css @@ -4692,6 +4692,52 @@ textarea.clone-input { box-shadow: var(--shadow-3); } +.pr-readiness { + position: relative; + margin-top: 15px; + min-height: 44px; + padding: 8px 10px; + border-block: 1px solid var(--border); + display: grid; + grid-template-columns: minmax(0, 1fr) auto; + align-items: center; + gap: 7px 18px; + background: color-mix(in oklch, var(--bg-elev), transparent 52%); +} +.pr-readiness::before { + content: ""; + position: absolute; + inset: -1px auto -1px 0; + width: 3px; + background: var(--text-dim); +} +.pr-readiness.ready::before { background: var(--add); } +.pr-readiness.blocked::before { background: var(--del); } +.pr-readiness.pending::before { background: var(--warn); } +.pr-readiness-main, +.pr-readiness-facts { min-width: 0; display: flex; align-items: center; } +.pr-readiness-main { gap: 8px; } +.pr-readiness-main > span:last-child { min-width: 0; display: flex; align-items: baseline; flex-wrap: wrap; gap: 4px 7px; } +.pr-readiness-main strong { color: var(--text); font-size: 11px; } +.pr-readiness-main small { color: var(--text-muted); font-size: 9px; } +.pr-readiness-icon { width: 20px; height: 20px; flex: none; display: grid; place-items: center; color: var(--text-muted); } +.pr-readiness.ready .pr-readiness-icon { color: var(--add); } +.pr-readiness.blocked .pr-readiness-icon { color: var(--del); } +.pr-readiness.pending .pr-readiness-icon { color: var(--warn); } +.pr-readiness-facts { justify-content: flex-end; flex-wrap: wrap; gap: 6px 12px; color: var(--text-muted); font-size: 9px; } +.pr-readiness-facts span { display: inline-flex; align-items: center; gap: 4px; white-space: nowrap; } +.pr-readiness-details { grid-column: 1 / -1; padding-top: 6px; border-top: 1px solid var(--border); color: var(--text-muted); font-size: 9px; } +.pr-readiness-details summary { width: fit-content; border-radius: var(--r-sm); color: var(--text-2); cursor: pointer; } +.pr-readiness-details summary:hover { color: var(--text); } +.pr-readiness-details summary:focus-visible { outline: 1px solid var(--accent); outline-offset: 3px; } +.pr-readiness-details ul { margin: 6px 0 1px; padding-left: 18px; display: grid; gap: 3px; } +.pr-readiness-details li::marker { color: var(--text-dim); } + +@media (max-width: 980px) { + .pr-readiness { grid-template-columns: 1fr; } + .pr-readiness-facts { justify-content: flex-start; } +} + .pr-pills { margin-top: 12px; display: flex; flex-wrap: wrap; gap: 6px; } .pr-pills > span:not(.pr-state) { padding: 2px 7px; border: 1px solid var(--border); border-radius: 999px; color: var(--text-muted); font-size: 9px; } @@ -5048,6 +5094,89 @@ textarea.clone-input { .pr-file-count { flex: none; padding: 9px 12px 7px; color: var(--text-muted); font-size: 9px; text-transform: uppercase; letter-spacing: .04em; } .pr-file-tree .tree-host-wrap { flex: 1 1 auto; min-height: 0; } .pr-diff-scroll { height: 100%; overflow: auto; background: var(--bg-base); } +.pr-diff-header { + position: sticky; + top: 0; + z-index: 4; + display: flex; + align-items: stretch; + min-width: 0; + background: var(--bg-panel); + border-bottom: 0.5px solid var(--border); +} +.pr-diff-header .pr-file-toggle { + position: static; + z-index: auto; + flex: 1 1 auto; + min-width: 0; + width: auto; + border-bottom: 0; +} +.pr-diff-tools { + flex: none; + display: flex; + align-items: center; + gap: 2px; + padding: 3px 7px 3px 4px; + border-left: 0.5px solid var(--border); +} +.pr-diff-tools .icon-btn:disabled { + opacity: .38; + cursor: default; +} +.pr-review-diff { + --diffs-annotation-min-height: 0; +} +.pr-inline-composer { + display: grid; + gap: 8px; + min-width: 320px; + padding: 10px; + background: var(--bg-elev); + border-block: 0.5px solid var(--accent); + font-family: var(--font-ui); +} +.pr-inline-composer-head, +.pr-inline-actions { + display: flex; + align-items: center; + gap: 10px; +} +.pr-inline-composer-head strong { + min-width: 0; + margin-right: auto; + overflow: hidden; + color: var(--text); + font-family: var(--font-mono); + font-size: 10.5px; + font-weight: 500; + text-overflow: ellipsis; + white-space: nowrap; +} +.pr-inline-composer textarea { + box-sizing: border-box; + width: 100%; + resize: vertical; + padding: 8px 9px; + border: 0.5px solid var(--border-strong); + border-radius: 5px; + outline: none; + background: var(--bg-base); + color: var(--text); + font: 11.5px/1.45 var(--font-ui); +} +.pr-inline-composer textarea:focus { border-color: var(--accent); } +.pr-inline-actions { justify-content: flex-end; } +.pr-inline-actions span { margin-right: auto; color: var(--text-dim); font-size: 9.5px; } +.pr-inline-message { + padding: 5px 10px; + border-bottom: 0.5px solid var(--border); + background: var(--bg-elev); + color: var(--text-muted); + font-size: 10.5px; +} +.pr-inline-message.ok { color: var(--add); } +.pr-inline-message.error { color: var(--del); } .pr-empty { flex: 1; diff --git a/ui/src/views/PullRequests.tsx b/ui/src/views/PullRequests.tsx index 9c6f805..22527b8 100644 --- a/ui/src/views/PullRequests.tsx +++ b/ui/src/views/PullRequests.tsx @@ -1,6 +1,6 @@ import { useCallback, useEffect, useMemo, useRef, useState } from 'react'; import { open as shellOpen } from '@tauri-apps/plugin-shell'; -import type { FileDiffMetadata } from '@pierre/diffs'; +import type { DiffLineAnnotation, FileDiffMetadata, SelectedLineRange } from '@pierre/diffs'; import type { GitStatusEntry } from '@pierre/trees'; import { Panel, PanelGroup, PanelResizeHandle } from 'react-resizable-panels'; @@ -14,7 +14,9 @@ import { diffStats, markdownUrl, parsePullRequestPatch, + pullRequestReadiness, pullRequestForBranch, + relativeTimeLabel, } from '../lib/pullRequests'; import { errMessage, tauri } from '../lib/tauri'; import { treeFileOrder } from '../lib/treeOrder'; @@ -393,14 +395,32 @@ function fileGitStatus(file: FileDiffMetadata): GitStatusEntry['status'] { return 'modified'; } -function PullRequestChanges({ path, pr }: { path: string; pr: PullRequest }) { +type InlineCommentAnnotation = { range: SelectedLineRange }; + +function PullRequestChanges({ + path, + provider, + pr, + onUpdated, +}: { + path: string; + provider: PullRequestList['repository']['provider']; + pr: PullRequest; + onUpdated: (next: PullRequest) => void; +}) { const diffMode = useSettings((state) => state.diffMode); + const platform = useSettings((state) => state.platform); + const setDiffMode = useRepo((state) => state.setDiffMode); const [patch, setPatch] = useState(null); const [loading, setLoading] = useState(true); const [error, setError] = useState(null); const [reload, setReload] = useState(0); const [selectedPath, setSelectedPath] = useState(null); const [collapsed, setCollapsed] = useState(false); + const [selectedLines, setSelectedLines] = useState(null); + const [commentDraft, setCommentDraft] = useState(''); + const [postingComment, setPostingComment] = useState(false); + const [commentMessage, setCommentMessage] = useState<{ tone: 'ok' | 'error'; text: string } | null>(null); const generation = useRef(0); useEffect(() => { @@ -442,7 +462,93 @@ function PullRequestChanges({ path, pr }: { path: string; pr: PullRequest }) { setSelectedPath((current) => current && filesByPath.has(current) ? current : treePaths[0] ?? null); }, [filesByPath, treePaths]); - useEffect(() => setCollapsed(false), [selectedPath]); + useEffect(() => { + setCollapsed(false); + setSelectedLines(null); + setCommentDraft(''); + setCommentMessage(null); + }, [selectedPath]); + + const openForReview = pr.state === 'open' || pr.state === 'active'; + const inlineCommentsSupported = provider === 'git_hub' && openForReview; + const inlineCommentTitle = provider !== 'git_hub' + ? 'Inline Azure DevOps comments need iteration tracking; open on host for now' + : openForReview + ? 'Comment on lines' + : 'This pull request is read-only'; + + const selectLines = useCallback((range: SelectedLineRange | null) => { + if (!inlineCommentsSupported || !range) { + setSelectedLines(null); + return; + } + const startSide = range.side ?? 'additions'; + const endSide = range.endSide ?? startSide; + if (startSide !== endSide) { + setSelectedLines({ start: range.end, end: range.end, side: endSide, endSide }); + return; + } + setSelectedLines({ + start: Math.min(range.start, range.end), + end: Math.max(range.start, range.end), + side: startSide, + endSide: startSide, + }); + setCommentMessage(null); + }, [inlineCommentsSupported]); + + const beginInlineComment = () => { + if (!selectedFile || !inlineCommentsSupported) return; + const hunk = selectedFile.hunks[0]; + if (!hunk) return; + const side = selectedFile.type === 'deleted' ? 'deletions' : 'additions'; + const line = side === 'deletions' ? hunk.deletionStart : hunk.additionStart; + selectLines({ start: Math.max(1, line), end: Math.max(1, line), side, endSide: side }); + requestAnimationFrame(() => document.getElementById(`pr-inline-comment-${pr.id}`)?.focus()); + }; + + const inlineAnnotations = useMemo[]>(() => { + if (!selectedLines) return []; + const side = selectedLines.endSide ?? selectedLines.side ?? 'additions'; + return [{ side, lineNumber: selectedLines.end, metadata: { range: selectedLines } }]; + }, [selectedLines]); + + const submitInlineComment = async () => { + if (!selectedFile || !selectedLines || postingComment) return; + const body = commentDraft.trim(); + if (!body) return; + const side = selectedLines.side ?? 'additions'; + setPostingComment(true); + setCommentMessage(null); + let posted = false; + try { + await tauri.repoPullRequestInlineComment( + path, + pr.id, + body, + selectedFile.name, + selectedLines.start, + selectedLines.end, + side, + pr.source_commit, + ); + posted = true; + setCommentDraft(''); + setSelectedLines(null); + const next = await tauri.repoPullRequest(path, pr.id); + onUpdated(next); + setCommentMessage({ tone: 'ok', text: 'Inline comment added.' }); + } catch (caught) { + setCommentMessage({ + tone: 'error', + text: posted + ? `Comment was added, but the pull request could not refresh: ${errMessage(caught)}` + : errMessage(caught), + }); + } finally { + setPostingComment(false); + } + }; const move = (delta: number) => { if (!treePaths.length) return; @@ -499,23 +605,106 @@ function PullRequestChanges({ path, pr }: { path: string; pr: PullRequest }) {
{selectedFile && ( <> - +
+ +
+ + + +
+
+ {commentMessage && ( +
+ {commentMessage.text} +
+ )} {!collapsed && ( - fileDiff={selectedFile} layout={diffMode === 'split' ? 'split' : 'unified'} hideFileHeader + className="pr-review-diff" + selectedLines={selectedLines} + lineAnnotations={inlineAnnotations} + onLineSelected={inlineCommentsSupported ? selectLines : undefined} + renderAnnotation={() => selectedLines ? ( +
{ + event.preventDefault(); + void submitInlineComment(); + }} + > +
+ + {selectedFile.name} · {selectedLines.side === 'deletions' ? 'old' : 'new'} line{selectedLines.start === selectedLines.end ? '' : 's'}{' '} + {selectedLines.start === selectedLines.end ? selectedLines.start : `${selectedLines.start}–${selectedLines.end}`} + + +
+