feat: replace gh pr merge subprocess with GraphQL mutation and add auto-merge status indicators#809
Open
robberwick wants to merge 22 commits intodlvhdr:mainfrom
Open
feat: replace gh pr merge subprocess with GraphQL mutation and add auto-merge status indicators#809robberwick wants to merge 22 commits intodlvhdr:mainfrom
robberwick wants to merge 22 commits intodlvhdr:mainfrom
Conversation
Add FinishedText field to TaskFinishedMsg to allow tasks to override their completion message based on the actual outcome. This enables more accurate feedback when task results vary (e.g., merge vs queue).
…uest type to PullRequestData - Add AllowMergeCommit, AllowSquashMerge, AllowRebaseMerge to Repository so the preferred merge method can be determined without an extra query - Add ID string to PullRequestData to carry the GraphQL node ID required as pullRequestId input for merge mutations - Add AutoMergeRequest struct and AutoMergeRequest field to PullRequestData to represent auto-merge configuration and support the visual indicator
…enabled PRs - Add AutoMergeIcon constant (timer with checkmark) - Update PR row to show auto-merge icon when AutoMergeRequest is set or AutoMergeEnabled flag is true - Update RenderState() to show "Auto-merge" text in PR details
…lper
Replace the two-step approach (subprocess + follow-up query) with a single
GraphQL mutation whose response directly reveals the outcome.
- Add mergeMethodForRepo() to select MERGE > SQUASH > REBASE based on what
the repository allows
- Add MergePullRequest() which picks the correct mutation based on
mergeStateStatus:
CLEAN/UNSTABLE → mergePullRequest (direct merge)
BLOCKED → addPullRequestToMergeQueue
default → enablePullRequestAutoMerge
and returns PRMergeStatus directly from the mutation response
…tation MergePR now calls data.MergePullRequest() directly via a goroutine instead of spawning a 'gh pr merge' subprocess followed by a follow-up status query. The mutation response is authoritative — no fallback 'assume merged' path. - Rewrite MergePR to use a plain func() tea.Msg goroutine - Add extractPullRequestData() helper and primaryPRDataProvider interface to retrieve the underlying *data.PullRequestData from the RowData value (handles both prrow.Data from the PR list and *data.PullRequestData from the branch section) - Add GetPrimaryPRData() to prrow.Data to satisfy the interface - Add IsQueued and AutoMergeEnabled fields to UpdatePRMsg
…nableAutoMerge helper
- Use sync.Mutex (clientMu) for goroutine-safe client initialisation
- Respect FF_MOCK_DATA feature flag to point the GraphQL client at localhost:3000
- Extract enableAutoMerge() helper encapsulating the enablePullRequestAutoMerge
mutation (reused by BLOCKED, BEHIND, and UNKNOWN/default cases)
- Expand mergeStateStatus switch:
CLEAN, UNSTABLE, HAS_HOOKS → mergePullRequest (direct merge)
DIRTY → error (merge conflicts)
BLOCKED, BEHIND → enableAutoMerge (enable auto-merge)
UNKNOWN → log warning, attempt enableAutoMerge
default → log warning, attempt enableAutoMerge
- Use package-level client (not cachedClient) for all mutations
…ction - prssection: on UpdatePRMsg with AutoMergeEnabled=true, set a non-nil AutoMergeRequest sentinel so the auto-merge icon renders immediately without waiting for a full data refresh - reposection: add tasks.UpdatePRMsg handler to Update() so merge results are reflected in the branch section - reposection/commands.go: update UpdatePRMsg doc comment
- Reject draft PRs before the network call with a clear error message - Remove IsQueued field from UpdatePRMsg (merge-queue support is not yet implemented; reserved for a future commit)
- Add internal/testhelpers/graphql package with LocalRoundTripper and NewMockGraphQLClient for use across test packages - Refactor internal/data/graphql_test_helpers_test.go to delegate to the new shared helpers, removing the inline localRoundTripper duplicate
- TestMergeMethodForRepo: priority order (MERGE > SQUASH > REBASE) and error for no allowed methods - TestMergePullRequest_*: direct merge (CLEAN/UNSTABLE/HAS_HOOKS), auto-merge (BLOCKED/BEHIND), DIRTY conflict error, and GraphQL error propagation - TestFF_MOCK_DATA_*: FF_MOCK_DATA feature flag routes to localhost:3000
… and AutoMergeEnabled - TestExtractPullRequestData_*: direct *PullRequestData, prrow.Data wrapper, nil primary provider, nil input, and unknown RowData types - TestMergePR_*: full MergePR task integration for direct merge, auto-merge (BLOCKED), auto-merge (BEHIND), draft PR guard, nil PR data error, and GraphQL error propagation - TestUpdatePRMsg_AutoMergeEnabled_SetsFlag: AutoMergeEnabled in UpdatePRMsg sets AutoMergeRequest sentinel in prssection so the icon renders immediately
The AutoMergeEnabled flag is a local UI state set when users enable auto-merge via the UI, not API-fetched data. Move it from PullRequestData to the prrow Data struct to properly separate concerns.
…ssertions PRMergeStatus.IsInMergeQueue was always false — the enableAutoMerge mutation does not return queue membership, and merge-queue mutation support is not yet implemented. Keeping the field would mislead callers into thinking the status is tracked. Remove it now and re-add alongside the actual merge-queue mutation when that work lands. The six assert.False(t, status.IsInMergeQueue) calls in prapi_test.go were tautological (asserting a zero value that could never be anything else), so they are removed along with the field.
…ientMu Both functions used a client == nil check-then-set pattern without holding clientMu, creating a data race with MergePullRequest (which already used the mutex) when called concurrently from goroutines. Wrap the lazy-init block in each function with clientMu.Lock/Unlock, matching the existing pattern in MergePullRequest. Add TestClientInit_ConcurrentCallsAreSafe to verify the fix under -race.
Add AutoMergeEnabled bool to data.PullRequestData as a local UI flag (not API-fetched) so the branch section can show the auto-merge icon immediately after the user triggers a merge, without waiting for a full refresh. Handle the field in reposection.Update's UpdatePRMsg case, mirroring the existing handler in prssection. Add reposection_test.go with three tests covering AutoMergeEnabled, IsMerged, and unknown-PR no-op cases.
…erState Add auto-merge icon rendering to branch.renderState() and branch.RenderState(): when a PR is open and either AutoMergeRequest is non-nil (set by the API) or AutoMergeEnabled is true (set locally after the user triggers auto-merge), the branch cell shows the AutoMergeIcon in warning colour, and RenderState returns '<icon> Auto-merge'. Also add a nil guard to RenderState so it returns an empty string when b.PR is nil, preventing a panic for branches without an associated PR. Add branch_test.go with 6 tests covering all RenderState paths including both auto-merge trigger sources.
…ate() end-to-end Replace the direct field-mutation approach with a call to m.Update() so the test exercises the full update path including BuildRows(). Introduce newFullTestModel() which uses the real NewModel() constructor to wire up Table, SearchBar, and PromptConfirmationBox with a fully-populated ProgramContext (Config, Theme, Styles), preventing panics during BuildRows(). Fix the type assertion from result.(Model) to result.(*Model): Update() returns section.Section wrapping a *Model (pointer receiver), so value-type assertion is a compile error.
Both cases use enableAutoMerge as an optimistic fallback rather than hard-failing. Add comments explaining the rationale: UNKNOWN means GitHub cannot determine merge state yet (checks pending, transient error), so auto-merge will fire once the condition clears. The default branch catches any future MergeStateStatus values added by GitHub. In both cases the log warning is intentionally log-only; surfacing it as a user-visible notification is deferred.
shurcooL-graphql reflects all exported fields of PullRequestData into the API query. AutoMergeEnabled is a local UI flag, not a GitHub API field, so without a tag it generated an 'autoMergeEnabled' selector that GitHub rejected with 'field doesn't exist on type PullRequest'. Add `graphql:"-"` to opt the field out of query generation. Expand the comment to explain why the field lives on the data-layer struct (branch.Branch holds *data.PullRequestData directly, with no UI-wrapper to carry UI flags) and flag it as a candidate for a future refactor toward a dedicated wrapper type analogous to prrow.Data.
shurcooL-graphql has no skip-field mechanism — the graphql:"-" tag caused GitHub to receive the literal string "-" as a field selector, rejecting the query at runtime. Remove AutoMergeEnabled from data.PullRequestData entirely. Instead: - Add AutoMergeEnabled bool to branch.Branch (UI-layer struct, never used as a GraphQL deserialization target). - Add autoMergeEnabledPRs map[int]bool to reposection.Model so the flag survives the scratch-rebuild inside updateBranchesWithPrs(). - UpdatePRMsg handler writes to the map; updateBranchesWithPrs re-applies it to each Branch after building from Prs. - Update branch.renderState/RenderState to read b.AutoMergeEnabled. - Update branch_test.go and reposection_test.go accordingly.
InitializeMarkdownStyle is only called when tea.BackgroundColorMsg arrives, which can happen after the first View() call that renders the PR sidebar. If GetMarkdownRenderer is called before that message arrives, dereferencing markdownStyle panics. Add a nil-check that falls back to the dark style, matching the behaviour InitializeMarkdownStyle would produce. The style will be overwritten as normal if BackgroundColorMsg subsequently arrives.
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.
Summary
Replaces the
gh pr mergesubprocess with a direct GraphQL mutation and wires up visual indicators so that auto-merge and merge-queue states are reflected immediately in the PR list and branch sections without waiting for a full data refresh.Core merge logic
MergePullRequestinspectsmergeStateStatusto decide between a direct merge and callingenableAutoMerge, and returns a typedPRMergeStatusso the caller knows which path was taken. TheMergePRtask reads merge-method settings from the PR data, dispatches anUpdatePRMsgcarryingIsMerged=trueorAutoMergeEnabled=truedepending on the outcome, and guards against attempting to merge a draft PR.Auto-merge status indicators
A new
AutoMergeIconis introduced. Both the PR row and the branch row checkAutoMergeRequest != nil(server-fetched) or the localAutoMergeEnabledflag (set optimistically after a successful enableAutoMerge call) and render the icon when either is true.AutoMergeEnabled flag placement
The flag cannot live on
data.PullRequestDatabecause that struct is used directly as a GraphQL deserialization target: theshurcooL-graphqllibrary writes every exported field into the query, so placing the flag there causes GitHub to reject the request. Instead, the flag is carried on the UI-layer wrapper types (prrow.Dataandbranch.Branch). The branch section uses anautoMergeEnabledPRs map[int]boolonreposection.Modelto persist the flag across the scratch-rebuild thatupdateBranchesWithPrsperforms on every update cycle.Concurrency fix
The GraphQL client lazy-init in
FetchPullRequestsandFetchPullRequestwas unprotected. A mutex is added to guard against concurrent calls.Nil-safe markdown rendering
This was a drive-by fix for a bug I encountered while testing.
markdownStyleis only set whentea.BackgroundColorMsgarrives. If the PR sidebar renders before that message is delivered,GetMarkdownRendererwould dereference a nil pointer and panic. A nil guard was added that falls back to the dark style as a safe default.How did you test this change?
MergePullRequest,mergeMethodForRepo, and theextractPullRequestDatahelper.MergePRtask using a shared GraphQL mock-client helper package (internal/testhelpers/graphql).branchandprrowpackages.prssectionUpdatePRMsgtest refactored to callm.Update()end-to-end with a fully wired context.reposectionUpdatePRMsgtests covering the auto-merge flag, a direct merge, and a no-op for an unknown PR number.go test -race ./internal/...passes cleanly.