Skip to content

feat(errors): pick the pull request to attach, and hydrate it on link - #642

Merged
Makisuo merged 1 commit into
mainfrom
feat/attach-pr-picker
Aug 26, 2026
Merged

Makisuo merged 1 commit into
mainfrom
feat/attach-pr-picker

Conversation

@Makisuo

@Makisuo Makisuo commented Aug 26, 2026 •

Copy link
Copy Markdown
Collaborator

Why

Attaching a PR is the hinge of the whole fix→verify loop in docs/error-issue-lifecycle.md — the link is what opens a post-merge verification window and lets Maple close an issue on evidence rather than on someone's word. That hinge was one text box asking for https://github.com/owner/repo/pull/123: leave Maple, find the PR, copy, come back, paste. Meanwhile the org's installation, repos and branches are already synced into vcs_*, and the issue knows its serviceName. Everything needed to just offer the right PR was already there and unused.

The bug this also fixes

linkPullRequest never called the provider. It string-parsed the URL and stored title: null, authorLogin: null, state: "open", and the row was only ever enriched when a pull_request webhook arrived.

So a PR that merged before it was attached — the normal human order of operations — never verified. GitHub does not redeliver a past closed event, onPullRequestEvent never fired, the verification window never opened, and the issue sat in in_review forever.

Merge handling is now factored out of onPullRequestEvent into a shared openMergedVerifications, called from the link path too.

What changed

Provider port — VcsProviderClient gains fetchPullRequests / fetchPullRequest, implemented over /pulls. One page, sort=updated&direction=desc, no pagination walk: the PR that fixes a live issue is recent by construction, and a walk would spend an installation's rate budget on history nobody scrolls to. Deliberately not /search/issues — its 30-req/min secondary limit is a poor fit for a keystroke-driven picker, so filtering happens client-side over the fetched page. state is normalized at the provider (merged_at != null ⇒ "merged"), the same rule the webhook mapper already applies.

Endpoint — GET /api/integrations/vcs/pull-requests?repository=&limit=, scoped to one connected repository so the picker's options can never cross a tenant boundary. An unconnected repo comes back as a 400, not a generic 404.

Hydration — crosses the VCS↔errors boundary through a new PullRequestLookup port, mirroring PullRequestEventSink but pointing the other way, so the errors side never imports the VCS layer. Strictly best-effort: an unconnected repo or an unreachable provider stores the link unenriched, exactly as today. A re-attach now also repairs a stale row whose webhook was missed, instead of the conflict path silently ignoring it.

Repo suggestion — rides on the existing listIssuePullRequests response, so no extra round trip. Four Postgres-only signals, first hit wins:

  1. a repo already linked to this issue
  2. the only connected repo
  3. an unambiguous serviceName ↔ repo-name match
  4. wherever this org's fixes most recently landed

It abstains rather than guessing when two repos match. A silent wrong preselection is worse than none — it is only noticed after somebody attaches the wrong PR. Resolving deployment.commit_sha → vcs_commits would be exact but puts a warehouse read on the issue page to compute a default; skipped on purpose.

UI — the dialog moves into attach-pull-request-dialog.tsx so three call sites can open it: the panel, the verification slot (when an in_progress/in_review issue has nothing attached), and after a transition to in_review. Pasting still works — full URL, #123, or a bare number — and stays the only way to attach a PR in a repo the org has not connected, which error_issue_pull_requests deliberately supports by not being FK'd to vcs_repositories.

Verification

bun typecheck and bun run lint clean. 140 tests pass across the touched suites, 11 of them new — including one asserting that attaching an already-merged PR opens a verification window, and one that the suggester abstains on an ambiguous match.

Browser pass against a locally seeded installation with three repos:

  • suggester returned the name match for a subscriptions-api issue, and for an integration-router issue correctly abstained and fell back to the org's most recent link (both confirmed from the API response, not the client fallback)
  • paste path attached; DB row confirmed stored with nulls when the provider was unreachable — the best-effort path, no regression
  • not-connected fallback renders the URL input plus a connect link
  • found and fixed a real defect: the new nudge button overflowed the 255px rail by 26px, now wraps

Not covered by the browser pass: a populated PR list against live data. There were no real App credentials locally, so vcs/pull-requests returned a correctly-mapped 502 throughout; that path rests on the unit tests.

For the reviewer

  • .oxlintrc.json gains one entry. apps/api/src/runtime/vcs-source-layer.ts is added to the vendor-import allowlist — it is a genuine composition root, and it exists so the alerting worker can get a source service without naming a provider module itself. Worth a look since it is a lint-config change.
  • Four composition roots now wire the new port. The webhook consumer (vcs-sync-runtime.ts) binds PullRequestLookup.none on purpose: it only handles deliveries that already carry the PR's title, state and merge, so nothing there asks a provider anything.

View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

Attaching a PR is the hinge of the fix→verify loop: the link is what opens a
post-merge verification window and lets an issue close on evidence. That hinge
was a bare text box asking for a PR URL, so the flow was leave Maple, find the
PR on GitHub, copy, come back, paste — while the org's repositories were
already synced and could simply have been offered.

Fixes a real bug along the way. linkPullRequest only string-parsed the URL and
stored title: null, state: "open", waiting for a webhook to enrich the row. For
a PR that merged BEFORE it was attached — the normal human order of operations
— no `closed` event is ever redelivered, so the verification window never
opened and the issue sat in `in_review` indefinitely. The merge handling is now
factored out of onPullRequestEvent into a shared openMergedVerifications and
called from the link path too.

- VcsProviderClient gains fetchPullRequests / fetchPullRequest, implemented
  over /pulls. One page, sort=updated, no pagination walk: the PR that fixes a
  live issue is recent by construction. Deliberately not /search/issues, whose
  30-req/min secondary limit suits a keystroke-driven picker poorly — filtering
  happens client-side over the fetched page.
- GET /api/integrations/vcs/pull-requests exposes them, scoped to one connected
  repository so the picker's options can never cross a tenant boundary.
- Hydration crosses the VCS↔errors boundary through a new PullRequestLookup
  port, mirroring PullRequestEventSink but pointing the other way, so errors
  never imports the VCS layer. Strictly best-effort: an unconnected repo or an
  unreachable provider stores the link unenriched, exactly as before. A
  re-attach also repairs a stale row whose webhook was missed.
- listIssuePullRequests now carries suggestedRepository, so the picker opens on
  the right repo with no extra round trip. Four Postgres-only signals, first hit
  wins: a repo already linked to this issue, the only connected repo, an
  unambiguous service-name match, then wherever the org's fixes most recently
  landed. It abstains rather than guess when two repos match — a silent wrong
  preselection is only noticed after somebody attaches the wrong PR.
- The dialog moves into its own component so the panel, the verification slot,
  and the in_review transition can all open it. Pasting still works (full URL,
  #123, or a bare number), which stays the only way to attach a PR in a repo the
  org has not connected — something the link table deliberately supports.

Verified locally against a seeded installation: the suggester resolves by name
match and falls back to the most recent link when nothing matches; the paste
path attaches and stores an unenriched row when the provider is unreachable;
the not-connected fallback renders. Also fixed the new nudge button overflowing
the 255px rail.

The new runtime/vcs-source-layer.ts is added to the vendor-import allowlist in
.oxlintrc.json — it is a composition root, which is what lets the alerting
worker get a source service without naming a provider module itself.
@Makisuo
Makisuo merged commit bddf153 into main Aug 26, 2026
41 checks passed
@Makisuo
Makisuo deleted the feat/attach-pr-picker branch August 26, 2026 11:45
@github-actions

Copy link
Copy Markdown
Contributor

🍁 Maple PR preview

Warning

Preview cleanup could not be confirmed. The Alchemy teardown outcome was skipped.

Final commit f09d5c3 · View workflow run

This branch had an error being deployed

1 failed deployment
pr-preview — f09d5c39 Deployed Aug 26, 2026 by Makisuo via deploy-pr-preview #1573
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant