feat: accept pull/merge request URLs in the repository field - #10
Open
jakob1379 wants to merge 6 commits into
Open
feat: accept pull/merge request URLs in the repository field#10jakob1379 wants to merge 6 commits into
jakob1379 wants to merge 6 commits into
Conversation
The repository path field (desktop app, web mode) and `diffcore analyze --repo` now take a PR/MR URL. Diffcore clones into ~/.diffcore/cache/repos/<host>/<owner>/<repo> (DIFFCORE_REPO_CACHE_DIR overrides), fetches the provider's pull-request ref namespace, and resolves to a base..head pair matching the provider's "Files changed" view. Supported via git refs alone, no API token: GitHub.com/GHE, GitLab SaaS/self-managed, Gitea/Forgejo/Codeberg/Gitee/Gogs, Bitbucket Data Center, Azure DevOps (Services/visualstudio.com/Server), Gerrit and SourceForge. Bitbucket Cloud, Launchpad, CodeCommit, Phabricator, SourceHut and Radicle publish no PR refs; their URLs parse but fail with a message naming the provider instead of a generic parse error. Detection keys off URL shape rather than hostname, so self-hosted instances work on any host, port or context path. Clone and fetch shell out to the git CLI: libgit2 is built here without network transports, and the CLI inherits the user's credential helper and SSH agent for private repositories. Base is reported as the fork point so a two-dot base..head diff equals the provider's view. Providers drop the merge ref once a PR lands, so for merged PRs the pre-merge tip is recovered from the merge commit that landed it; squash- and rebase-merged PRs leave no such commit and remain unresolvable without a provider API. See docs/pr-url-providers.md for the full provider table.
`diffcore-web` was never built: default.nix passes `--features diffcore-tauri/custom-protocol`, which implies `desktop`, and the bin is gated behind `required-features = ["web"]`. The two features are mutually exclusive (compile_error! in diffcore-tauri/src/lib.rs — the web dispatcher constructs the shim State, tauri commands need the real one), so they cannot share a derivation. Parameterize default.nix with `webMode` instead of duplicating it. `-p diffcore-tauri --no-default-features` is required: a bare `--features web` on the workspace lets cargo's feature unification pull `desktop` back in through the default and trips the compile_error. Desktop items and hicolor icons are dropped in web mode; both variants still install share/diffcore/ui, which diffcore-web resolves relative to its own binary. Exposes packages.diff-core-web and apps.web (`nix run .#web`).
Resolving a PR URL set repoPath/baseRef/headRef and called runAnalysis in the same tick, so every callback runAnalysis triggers still closed over the pre-resolution state. Selecting a file then sent the URL to get_file_diff, which canonicalizes it: Failed to load diff for tests/foo.py: IO error: Invalid repo path: No such file or directory (os error 2) The previous AnalyzeTarget override only patched the analyze call itself; get_file_diff, get_cached_refinement, the annotate/refine jobs and the comment commands all read the same stale state. Drop the override and queue the analysis behind a pendingAnalysis flag instead, so it starts on the next render with every callback rebuilt against the checkout.
Three independent reviews of the PR-URL feature. Acting on the findings. Two claims shipped in 7de27a8 were wrong: SourceForge was listed as supported. It is not, and cannot be: `git ls-remote https://git.code.sf.net/p/mingw/mingw-org-wsl 'refs/merge-requests/*'` returns nothing while a bare ls-remote on the same URL lists HEAD and branches, and Allura keeps the MR head in the submitter's fork, reachable only via REST. It now returns NoGitRefs and is documented as unsupported rather than failing with a message that blames the user's credentials. The documented "known gap" was backwards. Squash and rebase merges work fine — both rewrite the commit, so the head is not an ancestor of the default branch and the plain merge base is already the fork point. Fast-forward merges are what break, and they were failing silently: base collapsed onto head and the user got a successful review of zero files. Now an explicit EmptyDiff error. Correctness fixes: - Cached clones only ever fetched PR refs, so refs/remotes/origin/* was frozen at first-clone time and every merge base computed from the default branch drifted as the cache aged. Fetch +refs/heads/* --prune. - Cache directories slugged `/` to `-`, so gitlab.com/a/b/repo and gitlab.com/a-b/repo shared one checkout and served each other's diffs. Key on a hash of the clone URL, and realign origin on reuse. - loadRepoInfo unconditionally auto-detected base/head, overwriting the refs submitRepoInput had just resolved a few hundred ms earlier — the checkout is detached, so the selectors reverted to main/HEAD and any re-analysis diffed the wrong range. - Azure DevOps publishes only refs/pull/{n}/merge, but pick_refs required /head or /from, so Azure never resolved. Derive head from merge^2. - Gerrit anchored on `c` as the first path segment, so every instance mounted under a context path (Wikimedia, Eclipse, most corporate installs) failed to parse. Anchor on `+`, walk back to `c`, and keep the context in the clone URL — verified live that gerrit.wikimedia.org/r/mediawiki/core is the git URL and /mediawiki/core is not. - Route markers were matched left-to-right without checking the following segment, so a project named `pull-requests` or `pull` shadowed the real route. Scan right-to-left and require a number. - Serialize concurrent resolves with a lock file: they share one working tree, and a second resolve would flip HEAD under an in-flight analysis. - Suppress GUI askpass and SSH prompts, not just the terminal one, so a private repo fails fast instead of hanging behind an invisible dialog. Still no wall-clock timeout: a first clone of a monorepo legitimately takes minutes. Also: add Pagure (publishes refs/pull/{n}/head); drop GTK/WebKit from the headless nix closure; reuse config::diffcore_config_home; drop the unread provider/number fields from ResolvedPr; replace the module doc table with a link so it cannot drift from docs/pr-url-providers.md. Tests go from 7 to 14, covering each bug above against local fixtures. Both the origin-refresh and fast-forward cases were confirmed to fail without their fix.
…flow The E2E suite could not run anywhere. Locally the devshell supplied no browsers, so playwright fell back to an ambient PLAYWRIGHT_BROWSERS_PATH pointing at a store path that no longer existed; downloading browsers instead got past that but they die on launch, being unpatched for NixOS. CI never invoked playwright at all. The branch this rebased onto had just repaired 45 specs that nothing executed. Take the browsers from nixpkgs in the devshell. That requires the npm package and the nix driver to agree on a browser revision, so pin @playwright/test to exactly the driver's 1.61.1 — a caret range silently drifts to a revision the pinned browsers do not contain, which is the failure this started as. Bumping the lockfile moves npmDeps.hash too. CI keeps using npm's prebuilt browsers, which are fine on ubuntu, and now actually runs the suite, uploading the report on failure. Also cover the PR-URL flow, which was untestable by construction: demo mode short-circuited before resolve_pr_url, so the two bugs already found and fixed there shipped with no E2E hook. Demo mode now returns MOCK_RESOLVED_PR, whose refs deliberately differ from MOCK_REPO_INFO's branches, so auto-detection clobbering the resolved refs is observable. The first version of that spec asserted immediately after the input value settled and so raced loadRepoInfo's 100ms mock delay — it passed with the fix reverted, which makes it worthless. It now waits for the branch list to populate (proof loadRepoInfo has landed) before asserting the PR's refs survived, and was confirmed to fail without the guard. 209 passed, 2 skipped.
Three more independent reviews. The headline finding is that this branch
repeated the exact failure the first round was called in for: prose
asserting behaviour the code does not implement, and a new E2E spec that
passed with its fix fully reverted.
Prose corrected to match the code:
- docs claimed unsupported providers "still get a message naming the
provider". True for three of them; Phabricator, SourceHut, Radicle and
Google CSR have no Provider variant and fall through to a bare
"No such file or directory". The table is now split into recognised-
but-unsupported and not-recognised.
- docs still described the pre-21571e8 cache layout
(<host>/<owner>/<repo>); the code produces <host>/<repo>-<sha8>.
- "GIT_TERMINAL_PROMPT=0 ... fails fast instead of hanging" was false.
credential.helper runs before every askpass variable and blocks on a
locked keyring or biometric prompt; reproduced with a stock gh helper.
Documented as a real limitation instead.
- Azure DevOps was listed with a /head ref in one place and "merge only"
in another. It refuses anonymous ls-remote even on public projects, so
nobody here has observed its layout: the row is now marked inferred.
- The lock was described as protecting an in-flight analysis. It does not;
it is dropped when resolve returns. Claim removed, and the sharp edge
(all PRs of a repo share one worktree) left honestly documented.
Correctness:
- A default-branch rename left refs/remotes/origin/HEAD dangling: --prune
deletes the old branch but does not re-point HEAD, so default_branch
returned a dead ref and every PR without a merge ref became unresolvable
until the user deleted a cache directory the error never named. Now runs
`remote set-head origin --auto`, with a test that fails without it.
- AWS CodeCommit console URLs tripped the generic `pull-requests` marker
and were reported as Bitbucket Cloud. Given its own variant.
- Stale-lock reaping unlinked by path, so two processes could both reap and
the second would delete the first's fresh lock. Now claims a stale lock
by atomic rename. Dropped the 900s wait, which blocked a synchronous
Tauri command behind a spinner and was shorter than the staleness
threshold anyway, so waiters could never win.
- resolve_pr_url ran on the desktop main thread; a first monorepo clone
froze the window for minutes. Now async over spawn_blocking.
- --base/--head are overridden rather than filled in. export-groups and
import-groups pre-set base, so get_or_insert silently no-opped and the
PR head was diffed against the cached clone's own default branch.
- Gitee publishes refs/pull/{n}/MERGE; the merge-ref match was
case-sensitive. Matched case-insensitively.
Tests:
- The E2E spec claiming to pin b25ff68 passed with that fix fully reverted:
demo mode answers from mocks, so a stale repoPath is invisible in the UI.
The app now records what each backend call was parameterised with, and
the spec asserts on that. Verified it fails with the fix reverted,
reporting the PR URL where the checkout should be.
- Tests no longer set DIFFCORE_REPO_CACHE_DIR. set_var mutated the process
environment while 1700 other tests called getenv — a data race today and
a hard error in edition 2024. The cache root is a parameter now, which
also lets the traversal test assert through resolve()'s result rather
than the private cache_dir.
CI and tooling:
- The upload-artifact step uploaded nothing: reporter was "list", so
playwright-report/ was never written. Added the html reporter and traces.
- The E2E step ran before cargo test, so a UI flake stopped the Rust suite
reporting at all. Rust runs first now.
- A devshell warning catches @playwright/test drifting from
nixpkgs.playwright-driver, which nothing else couples.
2206 Rust tests pass; 208 E2E pass, 2 skipped.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
The repository field (desktop app, web mode) and
diffcore analyze --reponow accept apull/merge request URL in place of a filesystem path. Diffcore clones into a cache,
fetches the provider's PR ref namespace, and resolves the URL to a
base..headpairmatching what the provider shows under "Files changed". No API token is involved.
Detection keys off URL shape, not hostname, so self-hosted GitHub Enterprise, GitLab,
Gitea/Forgejo, Gogs and Bitbucket Data Center work on any host, port or context path.
Supported via git refs alone: GitHub.com/GHE, GitLab (SaaS + self-managed, nested
subgroups, pre-11.0 URLs), Gitea/Forgejo/Codeberg/Gitee/Gogs, Pagure, Bitbucket Data
Center, Azure DevOps, Gerrit, and GitHub REST URLs.
Recognised but unsupported (they publish no PR refs, so they fail with a message
naming the provider rather than a generic parse error): Bitbucket Cloud, SourceForge,
Launchpad, AWS CodeCommit.
Full table, including forges that aren't recognised at all and why:
docs/pr-url-providers.md.Also in this PR
diff-core-webis now packaged. It was never built:default.nixpassed--features diffcore-tauri/custom-protocol, which impliesdesktop, while the binaryis gated behind
required-features = ["web"]. The two features are mutually exclusive(
compile_error!inlib.rs), sodefault.nixis parameterised withwebModeratherthan duplicated. The headless closure is 13 store paths against the desktop build's 183,
with no GTK/WebKit.
supplied no browsers, and CI never invoked Playwright at all — including the 45 specs
repaired just before this branch. Browsers now come from
pkgs.playwright-driver.browsers,@playwright/testis pinned to exactly that driver'sversion, and CI runs the suite (after the Rust suite, so a UI flake can't hide it).
Verification
Known limitations
These are documented rather than hidden:
rewritten SHA, so the base branch is unrecoverable from git alone. Diffcore errors
explicitly rather than presenting a successful review of zero files. (Squash and rebase
merges are fine — both rewrite the commit.)
ls-remoteeven onpublic projects. The code treats a missing head ref as normal and derives the head from
the merge commit's second parent; that path is covered by a local fixture but has never
been run against a real instance. Worth one check with a PAT before trusting the row.
credential.helpercan still hang a resolve. It runs before everyaskpass variable, so suppressing prompts does not fully prevent blocking on a locked
keyring. There is deliberately no wall-clock timeout, since a first monorepo clone
legitimately takes minutes.
resolves, but resolving a second PR while an analysis of the first is in flight will
swap the tree under it.
Note for reviewers
ast::tests::test_large_file_performanceasserts a 500 ms wall-clock bound and ispre-existing and unrelated to this branch, but it fails under parallel load. Adding the
Playwright job to CI makes tripping it more likely.