reya/signed
1
0
Fork 0
Code Issues Pull Requests Actions 5 Releases Activity

improve pull request flow

Browse Source
This commit is contained in:
reya
2026-09-02 09:53:50 +07:00
parent c054f61593
commit db3eaff4b9
6 changed files with 1210 additions and 279 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/PLAN.md
-165
View File
@@ -1,165 +0,0 @@
# Plan
Two work streams:
1. **Fork support (display + navigation UI)** — show when a repository is a fork and let the user jump to the upstream repository.
2. **Pull request improvement** — bring PR creation/updating in line with the other NIP-34 clients (nak, ngit).
---
## 1. Fork support
### Background: what NIP-34 says about forks
NIP-34 has no fork event kind — a fork is an ordinary kind-30617 announcement by another author (or the same author under a different `d`). Fork-ness is expressed by two tags:
- **`u` tag** on the fork's announcement:
`["u", "30617:<upstream-pubkey>:<upstream-id>|<git-url>", "<relay-hint>", "<upstream-author-pubkey>"]`.
Including `u` means the author does **not** assert maintainership of the primary project (the fork is a *subordinate* of the upstream).
- **EUC** (`r` tag with `euc` marker): shared between the fork and its upstream (and other mirrors), so clients can group them. For a permanent fork, the EUC is the first commit after the fork point.
### Current state
- `Announcement::from_event` parses the `u` tag into an opaque string (`crates/signed_core/src/model.rs:212`; only the first value is kept, manually, because the SDK's `Nip34Tag` has no `Upstream` variant).
- `effective_maintainers` excludes the fork author (`model.rs:250`) — already correct per NIP-34.
- The About dialog shows the raw upstream string as a plain row (`crates/workspace/src/views/repo_detail/about.rs:68`).
- Nothing shows fork-ness in the repo list or the repo detail header, and there is no way to navigate to the upstream.
### Goal
- **Repo list card** (`crates/workspace/src/views/repo_list.rs::render_card`): show a "Forked from <upstream name>" badge instead of/in addition to the description, with a fork icon.
- **Repo detail header** (`crates/workspace/src/views/repo_detail/mod.rs::render_header`): show a "Forked from <name>" text button near the repo name.
- **Clicking the upstream** opens the upstream repository as a center panel (same as clicking any repo card).
### Design
#### 1.1 Structured `Upstream` model (`signed_core`)
Add a structured type and keep the manual parse:
```rust
pub struct Upstream {
/// `30617:<pubkey>:<id>` (navigable) or a git URL (not navigable).
pub target: UpstreamTarget,
pub relay_hint: Option<RelayUrl>,
pub author: Option<PublicKey>,
}
pub enum UpstreamTarget {
/// Parseable via the SDK `Coordinate` (`30617:<pubkey-hex>:<id>`).
Repo(RepoAddr),
/// Git https URL form: no NIP-34 announcement, not navigable.
GitUrl(Url),
}
```
- Change `Announcement.upstream: Option<String>` to `Option<Upstream>`; parse all three `u` values (the SDK's `Nip34Tag::parse` is not usable here — keep the manual `tag.kind() == "u"` branch and extend it).
- `RepoAddr` is the SDK `Coordinate` (`crates/signed_core/src/addr.rs`), so `Coordinate::from_str` gives the upstream address directly; validate it is kind `30617`.
- Update `about.rs` (renders `upstream`), `effective_maintainers`, and the `model.rs` tests (`parses_upstream_tag`, `effective_maintainers_exclude_owner_for_subordinate_forks`).
#### 1.2 Resolving the upstream announcement
Opening a panel needs an `Announcement` (`RepoDetailView::new`), so resolve the upstream announcement before (or while) opening:
1. **Lookup, no fetch**: the global `RepoListStore` holds every announcement in the local database (`crates/signed_state/src/repo_list.rs:51`). Look up the upstream `RepoAddr` there — covers the common case (upstream already browsed/known) with zero network.
2. **Miss → fetch, then open**: add a `Backend` method (e.g. `fetch_announcement(addr) -> Task<Result<Announcement>>`) doing a one-shot query with `filters::announcement(addr)` (`crates/signed_core/src/filters.rs:21`), mirroring the bootstrap fetch in `RepoStore::subscribe_remote` (`crates/signed_state/src/repo.rs:196`). Show the upstream as a disabled/loading row until it resolves; on failure fall back to showing the raw address.
3. **Git-URL upstreams**: not navigable — render as plain text with a copy action (like `copy_row` in `signed_ui`), no panel.
#### 1.3 Shared "open repo panel" helper
The open-panel sequence is currently duplicated three times:
- `crates/workspace/src/views/repo_list.rs:170` (`RepoListView::open_repo`)
- `crates/workspace/src/views/sidebar/mod.rs:155` (`SidebarPanel::open_repo`)
- `crates/workspace/src/views/sidebar/create_repo_dialog.rs:259` (`open_repo`)
Extract one helper (e.g. `open_repo_panel(dock_area, announcement, window, cx)` in the `workspace` views layer) and reuse it from all three plus the new fork button, so the fork navigation behaves exactly like clicking a repo card.
#### 1.4 Repo list badge
In `render_card`, when `announcement.upstream` is set:
- Resolve the upstream's display name via the `RepoListStore` lookup (1.2); fall back to the raw address string.
- Render a small "Forked from <name>" line (fork icon + `text_xs` muted), replacing or joining the description line. Add a `git-fork.svg` asset to `crates/assets/assets/icons/` + a `CustomIconName::GitFork` variant (lucide's `git-fork`), or reuse `git-branch.svg` if an asset addition is undesirable.
#### 1.5 Repo detail header
In `render_header` (`crates/workspace/src/views/repo_detail/mod.rs:1231`), next to the repo name:
- "Forked from <name>" as a **text button** (`gpui_base::Button` or styled `div`), which calls the shared open-panel helper with the resolved upstream announcement.
- Keep the About dialog row in sync: make it the same clickable control (or at least the same resolved display name).
- Handle "upstream not in store yet": spawn the `fetch_announcement` task; button shows a subtle loading state; on success open the panel (needs `window`/`cx` — the task is spawned on the view, `apply_announcement`-style flow).
#### 1.6 (Stretch) Fork grouping by EUC
`RepoListStore` already has `euc` per announcement; add a "N forks" count on the detail header by scanning announcements sharing the same EUC, with a filter or navigation into the explore list. Not required for the first iteration.
### Checklist
- [ ] `signed_core`: `Upstream`/`UpstreamTarget` types + full `u`-tag parse; `Announcement.upstream` type change; tests.
- [ ] `Backend::fetch_announcement(addr)` one-shot fetch.
- [ ] Shared `open_repo_panel` helper; switch the three existing call sites.
- [ ] Repo list card fork badge (+ `git-fork.svg` asset if used).
- [ ] Repo detail header "Forked from" button + About row sync.
- [ ] Manual test: fork with coordinate upstream (navigates), fork with git-URL upstream (copy only), upstream announcement absent (fetch-then-open).
---
## 2. Pull request improvement
### Why
Current PR creation (`RepoStore::open_pull_request`, `crates/signed_state/src/repo.rs:626`; dialog `crates/workspace/src/views/repo_detail/pull_requests.rs:288`) requires pasting `git format-patch` output, publishes no `merge-base`/`branch-name`, cannot update an existing PR, and advertises clone URLs the author usually cannot push to. Compared with nak (`pr send`/`pr update`/`pr merge`) and ngit (push-based PRs with merge-base inference), the gaps are:
| Area | Today | Fix (phase) |
| --- | --- | --- |
| Patch generation | manual paste | generate from a local checkout (P2) |
| `merge-base` tag | always `None` | compute vs state HEAD (P1) |
| `branch-name` tag | never | send local branch name (P1) |
| PR updates (kind 1619) | not producible | author-only update flow (P1) |
| Update reader trusts any author | `latest_update` has no author filter | filter by PR author (P1) |
| `clone` URL truthfulness | repo mirrors (author can't push) | push tip to grasp first (P3) |
| Multi-commit series | one oversized event | NIP-10 chain / size-aware (P3) |
| Pre-publish validation | none | `git am --check` dry run (P2) |
| Draft on create | no | optional 1633 status (P1) |
| Merge provenance | plain 1631 | `merge-commit`/`applied-as-commits` (P4) |
### Phase 1 — Correctness & interop (small, surgical) ✅ implemented
1. **Compute and publish `merge-base`, `branch-name`, `r` EUC** in `open_pull_request`:
- Target tip = `RepoStore.head` ref from the state announcement (`refs`/`head`, `crates/signed_state/src/repo.rs:28-30`); add `signed_git::merge_base(repo, a, b)` (shell out like `apply_patch`).
- Fill the `GitPullRequest` builder's existing `merge_base`/`branch_name` fields (currently hardcoded `None`, `repo.rs:691-702`); pass the branch name and tip through from the dialog.
- Add the `r` EUC tag manually to the PR event (the SDK builder omits it; NIP-34 recommends it for subscription efficiency).
2. **Add `RepoStore::update_pull_request(root, new_tip, …)`** producing a kind-1619 event via the SDK `GitPullRequestUpdate` builder (`E`/`P`/`K` NIP-22 tags) plus a chained root-revision patch (`t root-revision`, `e` reply to the original root patch). Author-only, mirroring nak's `pr update`. Wire a button into `pull_request_detail.rs`.
3. **Fix `latest_update`** (`crates/workspace/src/views/repo_detail/pull_request_detail.rs:1125`): filter by the root PR's author (nak and ngit both restrict tip updates to the PR author).
4. **Draft toggle** in the new-PR dialog: publish a 1633 status right after the PR event (reuse `set_status`).
**Status:** items 1 (partial — `branch-name` + `r` EUC done; `merge-base` remains `None` because the paste-based flow has no access to the author's git objects to compute a merge base; it becomes computable in Phase 2 when the patch is generated from a local checkout), 2, 3, 4 are implemented.
### Phase 2 — UX: replace the paste
5. **Local-repo picker** replaces the paste textarea (keep it as an advanced fallback): user picks a git checkout (or the app's `GitCache` mirror), source branch and target branch. The app then:
- resolves the tip (`git rev-parse`),
- computes `merge-base` vs the target tip,
- runs `format-patch base..tip --stdout` itself (add `signed_git::format_patch_between`, like nak),
- **dry-runs `git am --3way --check`** against the cached clone before publishing (`signed_git::apply_patch` infra, `crates/signed_git/src/lib.rs:176`), surfacing "does not apply" before anything hits the relays.
### Phase 3 — Truthful clone URLs (interop)
6. **Push before publishing**: add `signed_git::push_commit_ref(path, url, commit, ref)` and reuse the grasp-push infrastructure (`grasp_base_url`, `push_to_grasp_servers`, `crates/signed_state/src/backend.rs:1472,1500`) to push the tip to `refs/nostr/<event-id>` on the announced grasp servers (nak's `gitPushCommitToGraspRefs`). On success the `clone` tag carries the real URL; on failure fall back to the current patch-event model with a warning.
7. **Size-aware publishing**: split multi-commit mboxes into a NIP-10-chained 1617 series (each < 60 KB per NIP-34) or go PR-only above that size; adopt ngit's patch→PR upgrade (new PR + close-status for the original patch).
8. Optional: GRASP-06 `/prs/<npub>/<id>.git` + kind-10317 user grasp-list fallback (ngit's server-selection cascade). Fork support (section 1) makes the fork's own grasp server a natural push target here.
### Phase 4 — Merge provenance
9. In `merge_pull_request` (`crates/signed_state/src/repo.rs:817`), publish the 1631 status with `merge-commit` (or `applied-as-commits`) and `q` tags so nak/ngit/GitWorkshop show merge provenance correctly.
### Checklist
- [x] P1: merge-base + branch-name + `r` EUC on creation (merge-base deferred to P2 — not computable from a pasted patch).
- [x] P1: `update_pull_request` (1619) + UI button; author check.
- [x] P1: `latest_update` author filter.
- [x] P1: draft toggle on create.
- [ ] P2: local checkout picker + generated patch + pre-publish apply check.
- [ ] P3: push tip to grasp, truthful `clone` tags, size-aware series.
- [ ] P4: merge status tags.
docs/TODO.md
+13 -8
View File
@@ -2,19 +2,24 @@
## Fork support
- [x] Add UI for fork (see `PLAN.md` section 1):
- [x] Fork badge on repo list cards (`repo_list.rs::render_card`).
- [x] "Forked from …" text button in the repo detail header (`repo_detail/mod.rs::render_header`) and About dialog.
- [x] Clicking the upstream opens it as a center panel (shared `open_repo_panel` helper).
- [x] Fork badge on repo list cards (`repo_list.rs::render_card`).
- [x] "Forked from …" text button in the repo detail header (`repo_detail/mod.rs::render_header`) and About dialog.
- [x] Clicking the upstream opens it as a center panel (shared `open_repo_panel` helper).
## Pull request improvement (see `PLAN.md` section 2)
## Pull request improvement
- [x] P1: `branch-name` tag + `r` EUC tag on PR creation; draft checkbox in the new-PR dialog.
- [x] P1: `RepoStore::update_pull_request` (kind 1619 + root-revision patch) with an author-only "Update" button on the PR detail header.
- [x] P1: `latest_update` filters by PR author.
- [ ] P2: local checkout picker + generated patch + pre-publish apply check (also enables `merge-base`).
- [ ] P3: push tip to grasp, truthful `clone` tags, size-aware patch series.
- [ ] P4: `merge-commit`/`applied-as-commits` tags on merge status.
- [x] P2: local checkout picker in the new-PR dialog (folder picker + source/target branches + Generate): `signed_git::{merge_base, format_patch_between, patch_applies}`; `merge-base` tag now published; best-effort apply check shown under the patch field.
- [x] P3: push tip to grasp servers under `refs/nostr/<event-id>` before publishing (from the local checkout); multi-commit series published as NIP-10-chained 1617 events with a 60 KB per-patch cap; PR list shows dismissible error/warning banners (incl. push failures).
- [x] P4: merge status tags — `merge_pull_request` publishes 1631 with `applied-as-commits` + `r` per applied commit and `q`/`e`-reply tags per applied patch event.
### Pull request follow-ups
- [ ] GRASP-06 `/prs/<npub>/<id>.git` contributor endpoints + kind-10317 user grasp-list fallback.
- [ ] Merge button in the PR detail view (`merge_pull_request` is store-only today), then fetch-and-merge (`merge-commit`) when the push backend is guaranteed.
- [ ] Local-checkout generation for the update-PR dialog (currently paste-only).
## Performance: render path