Repository navigation
Conversation
Contributor
📝 WalkthroughWalkthroughThe PR retrieves repository merge capabilities, normalizes merge methods, updates both pull request merge menus, and rejects explicitly disabled methods before merging. It preserves merge behavior when capability retrieval fails. ChangesRepository merge settings
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant PRMergeMenu
participant GitOperationsRouter
participant RepoContext
participant GitHub
User->>PRMergeMenu: Open merge menu
PRMergeMenu->>GitOperationsRouter: Request repository merge settings
GitOperationsRouter->>RepoContext: Resolve and validate settings
RepoContext->>GitHub: Query repository capabilities
GitHub-->>RepoContext: Allowed methods and viewer default
RepoContext-->>GitOperationsRouter: Return settings
GitOperationsRouter-->>PRMergeMenu: Return settings
PRMergeMenu-->>User: Show ordered available methods
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Greptile SummaryThe PR centralizes GitHub merge-method selection and adds repository capability and default lookups for both desktop merge menus.
Confidence Score: 4/5The PR should not merge until the menus stop offering repository-disabled methods while capability settings are loading or unavailable. Both newly updated menus remain interactive before their settings queries resolve, and the shared fallback presents all merge methods, allowing users to submit requests that GitHub predictably rejects. Files Needing Attention: apps/desktop/src/renderer/lib/githubMergeMethods/githubMergeMethods.ts and both changed merge-menu components
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/lib/githubMergeMethods/githubMergeMethods.ts | Centralizes filtering and default ordering, but missing settings are treated as allowing every method. |
| apps/desktop/src/lib/trpc/routers/workspaces/utils/github/repo-context.ts | Adds CLI-backed merge-settings discovery and resolves known fork contexts against the upstream repository. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PRActionHeader/components/PRStatusGroup/PRStatusGroup.tsx | Fetches v2 repository capabilities but keeps the merge menu actionable before the query resolves. |
| apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/ChangesHeader/components/PRButton/PRButton.tsx | Applies shared merge-method selection to v1 while exposing the same unresolved-query fallback. |
| packages/host-service/src/trpc/router/github/github.ts | Preserves REST repository data and supplements it with a failure-tolerant GraphQL viewer default. |
| packages/host-service/test/integration/github-mocked.integration.test.ts | Updates mocked Octokit coverage for the added GraphQL request and returned default. |
Sequence Diagram
sequenceDiagram
participant User
participant Menu as Desktop merge menu
participant Settings as Repository settings query
participant GitHub
User->>Menu: Open merge options
Menu->>Settings: Fetch capabilities/default
alt Settings resolved
Settings-->>Menu: Allowed methods
Menu-->>User: Filtered methods
else Loading or failed
Menu-->>User: All three methods
User->>Menu: Select disabled method
Menu->>GitHub: Submit merge
GitHub-->>Menu: Reject method
end
Prompt To Fix All With AI
### Issue 1
apps/desktop/src/renderer/lib/githubMergeMethods/githubMergeMethods.ts:50-53
**Unresolved settings enable disabled methods**
When either repository-settings query is loading or has failed, this fallback exposes all three merge methods while both menus remain interactive, allowing users to submit repository-disabled methods that GitHub rejects.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "fix(desktop): respect repository merge s..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+50
to
+53
| ): MergeMethod[] { | ||
| if (!hasCompleteMergeSettings(settings)) { | ||
| return [...MERGE_METHODS]; | ||
| } |
Contributor
There was a problem hiding this comment.
When either repository-settings query is loading or has failed, this fallback exposes all three merge methods while both menus remain interactive, allowing users to submit repository-disabled methods that GitHub rejects.
Knowledge Base Used: Desktop App (Electron main + renderer)
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/lib/githubMergeMethods/githubMergeMethods.ts
Line: 50-53
Comment:
**Unresolved settings enable disabled methods**
When either repository-settings query is loading or has failed, this fallback exposes all three merge methods while both menus remain interactive, allowing users to submit repository-disabled methods that GitHub rejects.
**Knowledge Base Used:** [Desktop App (Electron main + renderer)](https://app.greptile.com/personal-org-6810/-/custom-context/knowledge-base/superset-sh/superset/-/docs/desktop-app.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 11 files
Architecture diagram
sequenceDiagram
participant V2 as "Workspace Sidebar (v2 PR merge menu)"
participant V1 as "Changes View (v1 PR merge menu)"
participant Shared as "Shared merge-method helper"
participant HostRouter as "Host-service githubRouter"
participant DesktopRouter as "Desktop changes router"
participant SettingsSvc as "getRepoMergeSettings util"
participant RepoCtx as "GitHub repo context helper"
participant GhCLI as "gh CLI"
participant GH as "GitHub REST/GraphQL APIs"
participant MergeAPI as "PR merge mutation"
Note over V2,GH: v2 flow: REST repo settings plus GraphQL viewer default
V2->>HostRouter: getRepo(owner, repo)
HostRouter->>GH: octokit.repos.get(owner, repo)
GH-->>HostRouter: allow_merge_commit / allow_rebase_merge / allow_squash_merge
HostRouter->>GH: octokit.graphql(viewerDefaultMergeMethod)
alt GraphQL default available
GH-->>HostRouter: viewerDefaultMergeMethod
else GraphQL unavailable/error
HostRouter->>HostRouter: Keep null default, warn
end
HostRouter-->>V2: Repo settings + viewerDefaultMergeMethod
Note over V1,GhCLI: v1 flow: gh repo view merge settings
V1->>DesktopRouter: getMergeSettings(worktreePath)
DesktopRouter->>SettingsSvc: getRepoMergeSettings(worktreePath)
SettingsSvc->>RepoCtx: getRepoContext(worktreePath)
RepoCtx-->>SettingsSvc: Repo context with fork upstream resolved
SettingsSvc->>GhCLI: gh repo view (origin/upstream args) --json merge,squash,rebase,viewerDefault
GhCLI-->>SettingsSvc: Merge settings JSON
alt Valid settings response
SettingsSvc->>SettingsSvc: Validate with Zod schema
SettingsSvc-->>DesktopRouter: Normalized merge settings
else Missing or invalid response
SettingsSvc-->>DesktopRouter: null
end
DesktopRouter-->>V1: Merge settings or null
Note over V1,Shared: Shared filtering and ordering
V2->>Shared: getAvailableMergeMethods(settings)
V1->>Shared: getAvailableMergeMethods(settings)
alt Complete settings
Shared->>Shared: Keep only allowed methods, move viewer default first if allowed
else Incomplete or null settings
Shared->>Shared: Keep squash, merge, rebase fallback order
end
Shared-->>V2: Allowed merge methods ordered
Shared-->>V1: Allowed merge methods ordered
Note over V2,MergeAPI: Menus render and submit only returned methods
V2->>MergeAPI: mergePR mutation with selected mergeMethod
V1->>MergeAPI: mergePR mutation with selected strategy
MergeAPI-->>V2: Merge result/toast
MergeAPI-->>V1: Merge result/toast
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
There was a problem hiding this comment.
2 issues found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/lib/trpc/routers/changes/git-operations.ts">
<violation number="1" location="apps/desktop/src/lib/trpc/routers/changes/git-operations.ts:322">
P3: Disabled-method enforcement has no desktop router test, so a future change could let a blocked strategy reach `mergePullRequest` or stop the intended fail-open behavior without detection. A focused test mocking `getRepoMergeSettings` for `false` and `null` responses would lock down both contracts.</violation>
</file>
<file name="packages/host-service/src/trpc/router/github/github.ts">
<violation number="1" location="packages/host-service/src/trpc/router/github/github.ts:14">
P3: This PR frames v1 and v2 around one shared method-selection helper, yet the "is this method disabled" enforcement is now implemented twice with independent field mappings: `isMergeMethodDisabled` in the host-service github router (snake_case REST fields: `allow_merge_commit`/`allow_squash_merge`/`allow_rebase_merge`) and the inline `methodDisabled` boolean chain in apps/desktop `git-operations.ts` mergePR (camelCase fields: `allowMergeCommit`/`allowSquashMerge`/`allowRebaseMerge`). Because the two copies live in separate packages and use differently-cased field names, a future change (e.g. a renamed capability or a new method) can easily make one merge menu show a method the other path rejects, or vice versa. Consider centralizing the method->capability mapping in one shared module so both routers resolve the same fields, keeping behavior consistent even though the two packages can't literally import the tRPC router.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| .mutation( | ||
| async ({ input }): Promise<{ success: boolean; mergedAt?: string }> => { | ||
| assertRegisteredWorktree(input.worktreePath); | ||
| const mergeSettings = await getRepoMergeSettings(input.worktreePath); |
Contributor
There was a problem hiding this comment.
P3: Disabled-method enforcement has no desktop router test, so a future change could let a blocked strategy reach mergePullRequest or stop the intended fail-open behavior without detection. A focused test mocking getRepoMergeSettings for false and null responses would lock down both contracts.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/lib/trpc/routers/changes/git-operations.ts, line 322:
<comment>Disabled-method enforcement has no desktop router test, so a future change could let a blocked strategy reach `mergePullRequest` or stop the intended fail-open behavior without detection. A focused test mocking `getRepoMergeSettings` for `false` and `null` responses would lock down both contracts.</comment>
<file context>
@@ -319,6 +319,21 @@ export const createGitOperationsRouter = () => {
.mutation(
async ({ input }): Promise<{ success: boolean; mergedAt?: string }> => {
assertRegisteredWorktree(input.worktreePath);
+ const mergeSettings = await getRepoMergeSettings(input.worktreePath);
+ const methodDisabled =
+ (input.strategy === "merge" &&
</file context>
| allow_squash_merge?: boolean | null; | ||
| } | ||
|
|
||
| function isMergeMethodDisabled( |
Contributor
There was a problem hiding this comment.
P3: This PR frames v1 and v2 around one shared method-selection helper, yet the "is this method disabled" enforcement is now implemented twice with independent field mappings: isMergeMethodDisabled in the host-service github router (snake_case REST fields: allow_merge_commit/allow_squash_merge/allow_rebase_merge) and the inline methodDisabled boolean chain in apps/desktop git-operations.ts mergePR (camelCase fields: allowMergeCommit/allowSquashMerge/allowRebaseMerge). Because the two copies live in separate packages and use differently-cased field names, a future change (e.g. a renamed capability or a new method) can easily make one merge menu show a method the other path rejects, or vice versa. Consider centralizing the method->capability mapping in one shared module so both routers resolve the same fields, keeping behavior consistent even though the two packages can't literally import the tRPC router.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/host-service/src/trpc/router/github/github.ts, line 14:
<comment>This PR frames v1 and v2 around one shared method-selection helper, yet the "is this method disabled" enforcement is now implemented twice with independent field mappings: `isMergeMethodDisabled` in the host-service github router (snake_case REST fields: `allow_merge_commit`/`allow_squash_merge`/`allow_rebase_merge`) and the inline `methodDisabled` boolean chain in apps/desktop `git-operations.ts` mergePR (camelCase fields: `allowMergeCommit`/`allowSquashMerge`/`allowRebaseMerge`). Because the two copies live in separate packages and use differently-cased field names, a future change (e.g. a renamed capability or a new method) can easily make one merge menu show a method the other path rejects, or vice versa. Consider centralizing the method->capability mapping in one shared module so both routers resolve the same fields, keeping behavior consistent even though the two packages can't literally import the tRPC router.</comment>
<file context>
@@ -1,6 +1,30 @@
+ allow_squash_merge?: boolean | null;
+}
+
+function isMergeMethodDisabled(
+ repository: RepositoryMergeCapabilities | null,
+ mergeMethod: MergeMethod,
</file context>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/lib/trpc/routers/changes/git-operations.merge.test.ts`:
- Around line 93-108: Add a test alongside “keeps the existing merge behavior
when settings are unavailable” that configures repository settings with
allowRebaseMerge set to null or omitted, then verifies caller.mergePR still
resolves successfully and mergePullRequestMock is called with the supplied
worktreePath and strategy. Ensure unknown capability values are treated as
enabled rather than blocking the merge.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fd3d5fba-3518-457a-a330-fe482871738f
📒 Files selected for processing (7)
apps/desktop/src/lib/trpc/routers/changes/git-operations.merge.test.tsapps/desktop/src/lib/trpc/routers/changes/git-operations.tsapps/desktop/src/renderer/lib/githubMergeMethods/githubMergeMethods.tspackages/host-service/src/trpc/router/github/github.tspackages/shared/package.jsonpackages/shared/src/github-merge-methods.test.tspackages/shared/src/github-merge-methods.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/desktop/src/lib/trpc/routers/changes/git-operations.ts
- packages/host-service/src/trpc/router/github/github.ts
2 of 3 tasks
abcdlsj
force-pushed
the
fix/issue-6185-merge-settings
branch
from
August 25, 2026 06:36
834b8e2 to
e50498a
Compare
This branch has not been deployed
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
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.
Design
Both pull request merge menus use the same pure merge-method selection logic and the same capability mapping.
@superset/shared/github-merge-methodsowns the supported method set, REST-to-canonical capability normalization, and explicit-disabled check.github.getRepo; v1 reads the equivalent fields throughgh repo view, resolving fork worktrees to their upstream repository.Validation
@sentry/electron/mainand@sentry/electron/renderermodules.Closes #6185
Summary by CodeRabbit
New Features
Bug Fixes
Tests