From 1ef0551bc138724d736583eac788f4183bef07e2 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Mon, 13 Jul 2026 20:10:50 -0700 Subject: [PATCH 01/17] Create pr not working for stacked worktree (#8651) * Fix stacked-worktree PR creation targeting a local-only parent branch - Resolve the eligibility default base to a remote-tracking ref instead of blindly trusting the submitted parent branch, since a stacked worktree's base is often a local-only branch the remote can't resolve - Add a create-time hard block (base_not_on_remote) so a stale or unpushed submitted base fails with actionable copy instead of the provider's opaque error - Update the dialog's default-base resolution and blocked-action/ dropdown copy to match the new remote-validated default * Split hosted-review-creation.test.ts to fix max-lines lint error Moved getHostedReviewCreationEligibility tests to a separate file (hosted-review-creation-eligibility.test.ts) to reduce the original file size from 880 to 579 lines, satisfying the max-lines lint constraint. Co-authored-by: Orca * Fix Create PR intent flow to use remote-validated eligibility default fo Prefer eligibilityDefaultBaseRef over the raw compare base when resolving the review base for the one-click Create PR intent flow, since eligibility is recomputed from the same compare base right before creation and already corrects a local-only stacked parent to the repo default. Falls back to the compare base only when eligibility supplies no default. * Simplify base-ref remote existence check into a single for-each-ref call Combine the wildcard and exact-tracking-ref lookups into one for-each-ref invocation with multiple patterns instead of two sequential git calls, removing the redundant rev-parse fallback path. --------- Co-authored-by: Orca --- src/cli/handlers/core.test.ts | 4 +- .../windows-firewall-remote-scope.test.ts | 10 +- ...hosted-review-creation-eligibility.test.ts | 529 ++++++++++++++++++ .../hosted-review-creation.test.ts | 282 ++-------- .../source-control/hosted-review-creation.ts | 108 +++- ...urce-control-create-pr-intent-flow.test.ts | 23 +- .../source-control-create-pr-intent-flow.ts | 11 +- ...ce-control-create-review-blocked-action.ts | 3 + .../source-control-dropdown-items.ts | 2 + ...control-primary-create-pr-intent-action.ts | 9 +- .../useCreatePullRequestDialogFields.test.ts | 28 +- .../useCreatePullRequestDialogFields.ts | 8 +- .../agent-completion-coordinator.test.ts | 21 +- ...gent-hook-completion-notifications.test.ts | 5 +- src/shared/hosted-review.ts | 4 + 15 files changed, 767 insertions(+), 280 deletions(-) create mode 100644 src/main/source-control/hosted-review-creation-eligibility.test.ts diff --git a/src/cli/handlers/core.test.ts b/src/cli/handlers/core.test.ts index 69f27dc9bd6..ac5444566d9 100644 --- a/src/cli/handlers/core.test.ts +++ b/src/cli/handlers/core.test.ts @@ -64,7 +64,9 @@ describe('orca claude-teams CLI handler', () => { spawnMock.mockImplementation(() => mockClaudeChild()) callMock.mockReset() callMock.mockResolvedValue({ - result: { launch: { env: { CLAUDE_CODE_EXPERIMENTAL_AGENT_TEAMS: '1', PATH: '/shim:/usr/bin' } } } + result: { + launch: { env: { CLAUDE_CODE_EXPERIMENTAL_AGENT_TEAMS: '1', PATH: '/shim:/usr/bin' } } + } }) previousRunAsNode = process.env.ELECTRON_RUN_AS_NODE previousPaneKey = process.env.ORCA_PANE_KEY diff --git a/src/main/runtime/windows-firewall-remote-scope.test.ts b/src/main/runtime/windows-firewall-remote-scope.test.ts index 2c9a013d964..6e2462c0334 100644 --- a/src/main/runtime/windows-firewall-remote-scope.test.ts +++ b/src/main/runtime/windows-firewall-remote-scope.test.ts @@ -126,16 +126,18 @@ describe('Windows firewall remote-address scope', () => { // A /32 subnet is just the desktop itself, so an explicit range cannot prove // the phone (a different host) is allowed — only the keywords can. expect(hasSufficientWindowsFirewallRemoteScope([rule(['Any'])], '100.64.1.20', 32)).toBe(true) - expect(hasSufficientWindowsFirewallRemoteScope([rule(['LocalSubnet'])], '100.64.1.20', 32)).toBe( - true - ) + expect( + hasSufficientWindowsFirewallRemoteScope([rule(['LocalSubnet'])], '100.64.1.20', 32) + ).toBe(true) expect( hasSufficientWindowsFirewallRemoteScope([rule(['100.64.0.0/10'])], '100.64.1.20', 32) ).toBe(false) }) it('fails address-family keywords closed when the interface family is unknown', () => { - expect(hasSufficientWindowsFirewallRemoteScope([rule(['Any'])], undefined, undefined)).toBe(true) + expect(hasSufficientWindowsFirewallRemoteScope([rule(['Any'])], undefined, undefined)).toBe( + true + ) expect(hasSufficientWindowsFirewallRemoteScope([rule(['Any4'])], undefined, undefined)).toBe( false ) diff --git a/src/main/source-control/hosted-review-creation-eligibility.test.ts b/src/main/source-control/hosted-review-creation-eligibility.test.ts new file mode 100644 index 00000000000..010d950d50f --- /dev/null +++ b/src/main/source-control/hosted-review-creation-eligibility.test.ts @@ -0,0 +1,529 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { + createGitHubPullRequestMock, + createGitLabMergeRequestMock, + createAzureDevOpsPullRequestMock, + createGiteaPullRequestMock, + isAzureDevOpsReviewCreationAuthenticatedMock, + isGiteaReviewCreationAuthenticatedMock, + getRepoSlugMock, + getProjectSlugMock, + getBitbucketRepoSlugMock, + getAzureDevOpsRepoSlugMock, + getGiteaRepoSlugMock, + getHostedReviewForBranchMock, + ghExecFileAsyncMock, + glabExecFileAsyncMock, + gitExecFileAsyncMock, + getUpstreamStatusMock, + getSshGitProviderMock, + getEnterpriseGitHubRepoSlugMock +} = vi.hoisted(() => ({ + createGitHubPullRequestMock: vi.fn(), + createGitLabMergeRequestMock: vi.fn(), + createAzureDevOpsPullRequestMock: vi.fn(), + createGiteaPullRequestMock: vi.fn(), + isAzureDevOpsReviewCreationAuthenticatedMock: vi.fn(), + isGiteaReviewCreationAuthenticatedMock: vi.fn(), + getRepoSlugMock: vi.fn(), + getProjectSlugMock: vi.fn(), + getBitbucketRepoSlugMock: vi.fn(), + getAzureDevOpsRepoSlugMock: vi.fn(), + getGiteaRepoSlugMock: vi.fn(), + getHostedReviewForBranchMock: vi.fn(), + ghExecFileAsyncMock: vi.fn(), + glabExecFileAsyncMock: vi.fn(), + gitExecFileAsyncMock: vi.fn(), + getUpstreamStatusMock: vi.fn(), + getSshGitProviderMock: vi.fn(), + getEnterpriseGitHubRepoSlugMock: vi.fn() +})) + +vi.mock('../github/client', () => ({ + createGitHubPullRequest: createGitHubPullRequestMock, + getRepoSlug: getRepoSlugMock, + getPRForBranch: vi.fn() +})) + +vi.mock('../github/github-enterprise-repository', () => ({ + getEnterpriseGitHubRepoSlug: getEnterpriseGitHubRepoSlugMock +})) + +vi.mock('../gitlab/client', () => ({ + getProjectSlug: getProjectSlugMock, + getMergeRequestForBranch: vi.fn(), + getMergeRequest: vi.fn() +})) + +vi.mock('../gitlab/merge-request-creation', () => ({ + createGitLabMergeRequest: createGitLabMergeRequestMock +})) + +vi.mock('../bitbucket/client', () => ({ + getBitbucketRepoSlug: getBitbucketRepoSlugMock, + getBitbucketPullRequestForBranch: vi.fn(), + getBitbucketPullRequest: vi.fn() +})) + +vi.mock('../azure-devops/client', () => ({ + getAzureDevOpsRepoSlug: getAzureDevOpsRepoSlugMock, + getAzureDevOpsPullRequestForBranch: vi.fn(), + getAzureDevOpsPullRequest: vi.fn() +})) + +vi.mock('../azure-devops/pull-request-creation', () => ({ + createAzureDevOpsPullRequest: createAzureDevOpsPullRequestMock, + isAzureDevOpsReviewCreationAuthenticated: isAzureDevOpsReviewCreationAuthenticatedMock +})) + +vi.mock('../gitea/client', () => ({ + getGiteaRepoSlug: getGiteaRepoSlugMock, + getGiteaPullRequestForBranch: vi.fn(), + getGiteaPullRequest: vi.fn() +})) + +vi.mock('../gitea/pull-request-creation', () => ({ + createGiteaPullRequest: createGiteaPullRequestMock, + isGiteaReviewCreationAuthenticated: isGiteaReviewCreationAuthenticatedMock +})) + +vi.mock('../github/gh-utils', () => ({ + acquire: vi.fn(), + release: vi.fn(), + ghExecFileAsync: ghExecFileAsyncMock, + gitExecFileAsync: gitExecFileAsyncMock +})) + +vi.mock('../gitlab/gl-utils', () => ({ + acquire: vi.fn(), + release: vi.fn(), + glabExecFileAsync: glabExecFileAsyncMock, + glabRepoExecOptions: (repoPath: string, connectionId?: string | null) => + connectionId ? {} : { cwd: repoPath } +})) + +vi.mock('../git/upstream', () => ({ + getUpstreamStatus: getUpstreamStatusMock +})) + +vi.mock('../providers/ssh-git-dispatch', () => ({ + getSshGitProvider: getSshGitProviderMock +})) + +vi.mock('./hosted-review', () => ({ + getHostedReviewForBranch: getHostedReviewForBranchMock +})) + +import { getHostedReviewCreationEligibility } from './hosted-review-creation' + +function resetMocks(): void { + for (const mock of [ + createGitHubPullRequestMock, + createGitLabMergeRequestMock, + createAzureDevOpsPullRequestMock, + createGiteaPullRequestMock, + isAzureDevOpsReviewCreationAuthenticatedMock, + isGiteaReviewCreationAuthenticatedMock, + getRepoSlugMock, + getProjectSlugMock, + getBitbucketRepoSlugMock, + getAzureDevOpsRepoSlugMock, + getGiteaRepoSlugMock, + getHostedReviewForBranchMock, + ghExecFileAsyncMock, + glabExecFileAsyncMock, + gitExecFileAsyncMock, + getUpstreamStatusMock, + getSshGitProviderMock, + getEnterpriseGitHubRepoSlugMock + ]) { + mock.mockReset() + } +} + +function mockGitHubProvider(): void { + getProjectSlugMock.mockResolvedValue(null) + getRepoSlugMock.mockResolvedValue({ owner: 'acme', repo: 'orca' }) + getBitbucketRepoSlugMock.mockResolvedValue(null) + getAzureDevOpsRepoSlugMock.mockResolvedValue(null) + getGiteaRepoSlugMock.mockResolvedValue(null) + getEnterpriseGitHubRepoSlugMock.mockResolvedValue(null) +} + +// GHES: github.com-only slug parsing misses the custom host, so the enterprise +// resolver claims the repo and reports the host for the gh auth probe (#8312). +function mockGitHubEnterpriseProvider(): void { + getProjectSlugMock.mockResolvedValue(null) + getRepoSlugMock.mockResolvedValue(null) + getBitbucketRepoSlugMock.mockResolvedValue(null) + getAzureDevOpsRepoSlugMock.mockResolvedValue(null) + getGiteaRepoSlugMock.mockResolvedValue(null) + getEnterpriseGitHubRepoSlugMock.mockResolvedValue({ + owner: 'acme', + repo: 'orca', + host: 'github.acme-corp.com' + }) +} + +function mockGitLabProvider(): void { + getProjectSlugMock.mockResolvedValue({ host: 'gitlab.com', path: 'acme/orca' }) + getRepoSlugMock.mockResolvedValue(null) + getBitbucketRepoSlugMock.mockResolvedValue(null) + getAzureDevOpsRepoSlugMock.mockResolvedValue(null) + getGiteaRepoSlugMock.mockResolvedValue(null) +} + +function mockAzureDevOpsProvider(): void { + getProjectSlugMock.mockResolvedValue(null) + getRepoSlugMock.mockResolvedValue(null) + getBitbucketRepoSlugMock.mockResolvedValue(null) + getAzureDevOpsRepoSlugMock.mockResolvedValue({ + host: 'dev.azure.com', + project: 'Project', + repository: 'orca', + apiBaseUrl: 'https://dev.azure.com/acme/Project', + webBaseUrl: 'https://dev.azure.com/acme/Project/_git/orca' + }) + getGiteaRepoSlugMock.mockResolvedValue(null) +} + +function mockGiteaProvider(): void { + getProjectSlugMock.mockResolvedValue(null) + getRepoSlugMock.mockResolvedValue(null) + getBitbucketRepoSlugMock.mockResolvedValue(null) + getAzureDevOpsRepoSlugMock.mockResolvedValue(null) + getGiteaRepoSlugMock.mockResolvedValue({ + host: 'git.example.com', + owner: 'acme', + repo: 'orca', + apiBaseUrl: 'https://git.example.com/api/v1', + webBaseUrl: 'https://git.example.com' + }) +} + +describe('getHostedReviewCreationEligibility', () => { + beforeEach(() => { + resetMocks() + + mockGitHubProvider() + getHostedReviewForBranchMock.mockResolvedValue(null) + ghExecFileAsyncMock.mockResolvedValue({ stdout: '', stderr: '' }) + gitExecFileAsyncMock.mockResolvedValue({ stdout: 'Feature title\n', stderr: '' }) + isAzureDevOpsReviewCreationAuthenticatedMock.mockReturnValue(true) + isGiteaReviewCreationAuthenticatedMock.mockReturnValue(true) + }) + + it('treats short remote base refs as the default branch name', async () => { + await expect( + getHostedReviewCreationEligibility({ + repoPath: '/repo', + branch: 'main', + base: 'origin/main', + hasUncommittedChanges: false, + hasUpstream: true, + ahead: 0, + behind: 0 + }) + ).resolves.toMatchObject({ + canCreate: false, + blockedReason: 'default_branch', + defaultBaseRef: 'origin/main' + }) + }) + + // Stacked-worktree base resolution (Change 1/2). `stackedArgs` defaults to a + // bare local-only parent; `mockRefs` controls the remote-tracking snapshot. + const stackedArgs = ( + overrides: Partial[0]> = {} + ): Parameters[0] => ({ + repoPath: '/repo', + branch: 'feature/stacked', + base: 'stacked-parent', + hasUncommittedChanges: false, + hasUpstream: true, + ahead: 0, + behind: 0, + ...overrides + }) + + const mockRefs = (opts: { + symbolicRef?: string + forEachRef?: string + forEachThrows?: boolean + revParseThrows?: boolean + }): void => { + gitExecFileAsyncMock.mockImplementation(async (args: string[]) => { + if (args[0] === 'symbolic-ref') { + return { stdout: opts.symbolicRef ?? '', stderr: '' } + } + if (args[0] === 'for-each-ref') { + if (opts.forEachThrows) { + throw new Error('ssh: connect: connection refused') + } + return { stdout: opts.forEachRef ?? '', stderr: '' } + } + if (args[0] === 'rev-parse' && opts.revParseThrows) { + throw new Error('unknown revision') + } + return { stdout: 'refs/remotes/origin/main\n', stderr: '' } + }) + } + + it('falls back to the repo default when a stacked parent base is local-only', async () => { + mockRefs({ symbolicRef: 'refs/remotes/origin/main\n' }) + await expect(getHostedReviewCreationEligibility(stackedArgs())).resolves.toMatchObject({ + canCreate: true, + blockedReason: null, + defaultBaseRef: 'origin/main' + }) + }) + + it('preserves a stacked parent base that exists on the remote', async () => { + mockRefs({ forEachRef: 'refs/remotes/origin/parent-pushed\n' }) + await expect( + getHostedReviewCreationEligibility(stackedArgs({ base: 'parent-pushed' })) + ).resolves.toMatchObject({ + canCreate: true, + blockedReason: null, + defaultBaseRef: 'parent-pushed' + }) + }) + + it('keeps the candidate base when no repo default can be resolved', async () => { + mockRefs({ revParseThrows: true }) + await expect(getHostedReviewCreationEligibility(stackedArgs())).resolves.toMatchObject({ + canCreate: true, + blockedReason: null, + defaultBaseRef: 'stacked-parent' + }) + }) + + it('preserves the candidate base when the remote probe cannot reach the host', async () => { + // Transport failure must not be read as "absent" — that would demote a + // legitimately-pushed parent to the repo default on a transient SSH blip. + mockRefs({ forEachThrows: true }) + await expect( + getHostedReviewCreationEligibility(stackedArgs({ base: 'parent-pushed' })) + ).resolves.toMatchObject({ canCreate: true, defaultBaseRef: 'parent-pushed' }) + }) + + it('blocks dirty tracked GitHub branches before PR creation', async () => { + await expect( + getHostedReviewCreationEligibility({ + repoPath: '/repo', + branch: 'feature/create-pr', + base: 'main', + hasUncommittedChanges: true, + hasUpstream: true, + ahead: 0, + behind: 0 + }) + ).resolves.toMatchObject({ + provider: 'github', + canCreate: false, + blockedReason: 'dirty', + nextAction: 'commit', + head: 'feature/create-pr' + }) + }) + + it('keeps dirty feature branches eligible for PR preparation when review lookup fails', async () => { + getHostedReviewForBranchMock.mockRejectedValueOnce(new Error('gh lookup failed')) + + await expect( + getHostedReviewCreationEligibility({ + repoPath: '/repo', + branch: 'feature/create-pr', + base: 'main', + hasUncommittedChanges: true, + hasUpstream: false, + ahead: 0, + behind: 0 + }) + ).resolves.toMatchObject({ + provider: 'github', + canCreate: false, + blockedReason: 'dirty', + nextAction: 'commit', + head: 'feature/create-pr' + }) + }) + + it('enables creation for clean, in-sync, authenticated GitHub feature branches', async () => { + await expect( + getHostedReviewCreationEligibility({ + repoPath: '/repo', + branch: 'refs/heads/feature/create-pr', + base: 'origin/main', + hasUncommittedChanges: false, + hasUpstream: true, + ahead: 0, + behind: 0 + }) + ).resolves.toMatchObject({ + provider: 'github', + canCreate: true, + blockedReason: null, + nextAction: null, + defaultBaseRef: 'origin/main', + head: 'feature/create-pr' + }) + }) + + it('detects a GitHub Enterprise Server branch as the GitHub provider (#8312)', async () => { + mockGitHubEnterpriseProvider() + + await expect( + getHostedReviewCreationEligibility({ + repoPath: '/repo', + branch: 'feature/create-pr', + base: 'origin/main', + hasUncommittedChanges: false, + hasUpstream: true, + ahead: 0, + behind: 0 + }) + ).resolves.toMatchObject({ + provider: 'github', + canCreate: true, + blockedReason: null, + nextAction: null + }) + + // Enterprise auth was already confirmed during detection; the gate must not + // fire a redundant gh probe. + expect(ghExecFileAsyncMock).not.toHaveBeenCalled() + }) + + it('resolves remote eligibility through SSH repo metadata without generating PR copy', async () => { + const remoteGit = { + exec: vi.fn(async () => ({ stdout: '', stderr: '' })) + } + getSshGitProviderMock.mockReturnValue(remoteGit) + + await expect( + getHostedReviewCreationEligibility({ + repoPath: '/remote/repo', + connectionId: 'ssh-1', + branch: 'feature/create-pr', + base: 'origin/main', + hasUncommittedChanges: false, + hasUpstream: true, + ahead: 0, + behind: 0 + }) + ).resolves.toMatchObject({ + provider: 'github', + canCreate: true, + head: 'feature/create-pr' + }) + + expect(getProjectSlugMock).toHaveBeenCalledWith('/remote/repo', 'ssh-1') + expect(getRepoSlugMock).toHaveBeenCalledWith('/remote/repo', 'ssh-1') + expect(getHostedReviewForBranchMock).toHaveBeenCalledWith( + expect.objectContaining({ repoPath: '/remote/repo', connectionId: 'ssh-1' }) + ) + // Why: the base-on-remote probe must run on the SSH host that will execute + // the provider create, so it flows through the relay exec, not local git. + expect(remoteGit.exec).toHaveBeenCalledWith( + ['for-each-ref', '--count=1', '--format=%(refname)', 'refs/remotes/*/main'], + '/remote/repo' + ) + }) + + it('offers push as the next action for authenticated branches with local-only commits', async () => { + await expect( + getHostedReviewCreationEligibility({ + repoPath: '/repo', + branch: 'feature/create-pr', + base: 'main', + hasUncommittedChanges: false, + hasUpstream: true, + ahead: 2, + behind: 0 + }) + ).resolves.toMatchObject({ + canCreate: false, + blockedReason: 'needs_push', + nextAction: 'push' + }) + }) + + it('enables creation for clean, in-sync, authenticated GitLab feature branches', async () => { + mockGitLabProvider() + + await expect( + getHostedReviewCreationEligibility({ + repoPath: '/repo', + branch: 'feature/gitlab', + base: 'main', + hasUncommittedChanges: false, + hasUpstream: true, + ahead: 0, + behind: 0 + }) + ).resolves.toMatchObject({ + provider: 'gitlab', + canCreate: true, + blockedReason: null, + nextAction: null, + head: 'feature/gitlab' + }) + expect(ghExecFileAsyncMock).not.toHaveBeenCalled() + expect(glabExecFileAsyncMock).toHaveBeenCalledWith( + ['auth', 'status', '--hostname', 'gitlab.com'], + { cwd: '/repo' } + ) + }) + + it('enables creation for clean, in-sync, token-configured Azure DevOps feature branches', async () => { + mockAzureDevOpsProvider() + + await expect( + getHostedReviewCreationEligibility({ + repoPath: '/repo', + branch: 'feature/azure', + base: 'main', + hasUncommittedChanges: false, + hasUpstream: true, + ahead: 0, + behind: 0 + }) + ).resolves.toMatchObject({ + provider: 'azure-devops', + canCreate: true, + blockedReason: null, + nextAction: null, + head: 'feature/azure' + }) + expect(isAzureDevOpsReviewCreationAuthenticatedMock).toHaveBeenCalledOnce() + expect(ghExecFileAsyncMock).not.toHaveBeenCalled() + expect(glabExecFileAsyncMock).not.toHaveBeenCalled() + }) + + it('enables creation for clean, in-sync, token-configured Gitea feature branches', async () => { + mockGiteaProvider() + + await expect( + getHostedReviewCreationEligibility({ + repoPath: '/repo', + branch: 'feature/gitea', + base: 'main', + hasUncommittedChanges: false, + hasUpstream: true, + ahead: 0, + behind: 0 + }) + ).resolves.toMatchObject({ + provider: 'gitea', + canCreate: true, + blockedReason: null, + nextAction: null, + head: 'feature/gitea' + }) + expect(isGiteaReviewCreationAuthenticatedMock).toHaveBeenCalledOnce() + expect(ghExecFileAsyncMock).not.toHaveBeenCalled() + expect(glabExecFileAsyncMock).not.toHaveBeenCalled() + }) +}) diff --git a/src/main/source-control/hosted-review-creation.test.ts b/src/main/source-control/hosted-review-creation.test.ts index 3ae82fd4010..04c751d7c3b 100644 --- a/src/main/source-control/hosted-review-creation.test.ts +++ b/src/main/source-control/hosted-review-creation.test.ts @@ -115,7 +115,7 @@ vi.mock('./hosted-review', () => ({ getHostedReviewForBranch: getHostedReviewForBranchMock })) -import { createHostedReview, getHostedReviewCreationEligibility } from './hosted-review-creation' +import { createHostedReview } from './hosted-review-creation' function resetMocks(): void { for (const mock of [ @@ -223,6 +223,11 @@ describe('createHostedReview', () => { if (args[0] === 'status') { return { stdout: '', stderr: '' } } + // Why: base-on-remote probe (Change 2 enforcement) — the default base + // resolves to a remote-tracking branch so create-time validation passes. + if (args[0] === 'for-each-ref') { + return { stdout: 'refs/remotes/origin/main\n', stderr: '' } + } if (args[0] === 'log' && args.includes('--pretty=%s')) { return { stdout: 'Feature title\n', stderr: '' } } @@ -301,6 +306,32 @@ describe('createHostedReview', () => { expect(createGitHubPullRequestMock).not.toHaveBeenCalled() }) + it('blocks creation with actionable copy when the submitted base is local-only', async () => { + // for-each-ref falls through to '' → the submitted stacked parent is not on + // the remote, so create-time enforcement blocks with actionable copy. + gitExecFileAsyncMock.mockImplementation(async (args: string[]) => { + if (args[0] === 'rev-parse') { + return { stdout: 'feature\n', stderr: '' } + } + return { stdout: '', stderr: '' } + }) + + await expect( + createHostedReview('/repo', { + provider: 'github', + base: 'stacked-parent', + head: 'feature', + title: 'Feature' + }) + ).resolves.toEqual({ + ok: false, + code: 'validation', + error: + 'Create PR failed: the base branch "stacked-parent" hasn\'t been pushed to the remote. Choose a pushed base or push it first.' + }) + expect(createGitHubPullRequestMock).not.toHaveBeenCalled() + }) + it('creates the pull request after fresh main-process validation passes', async () => { await expect( createHostedReview('/repo', { @@ -501,6 +532,10 @@ describe('createHostedReview', () => { if (args[0] === 'rev-parse' && args[1] === '--abbrev-ref' && args[2] === 'HEAD') { return { stdout: 'feature\n', stderr: '' } } + if (args[0] === 'for-each-ref') { + // Base-on-remote probe (Change 2) runs on the SSH host; base is pushed. + return { stdout: 'refs/remotes/origin/main\n', stderr: '' } + } if (args[0] === 'log' && args.includes('--pretty=%s')) { return { stdout: 'Feature title\n', stderr: '' } } @@ -588,248 +623,3 @@ describe('createHostedReview', () => { expect(createGitHubPullRequestMock).not.toHaveBeenCalled() }) }) - -describe('getHostedReviewCreationEligibility', () => { - beforeEach(() => { - resetMocks() - - mockGitHubProvider() - getHostedReviewForBranchMock.mockResolvedValue(null) - ghExecFileAsyncMock.mockResolvedValue({ stdout: '', stderr: '' }) - gitExecFileAsyncMock.mockResolvedValue({ stdout: 'Feature title\n', stderr: '' }) - isAzureDevOpsReviewCreationAuthenticatedMock.mockReturnValue(true) - isGiteaReviewCreationAuthenticatedMock.mockReturnValue(true) - }) - - it('treats short remote base refs as the default branch name', async () => { - await expect( - getHostedReviewCreationEligibility({ - repoPath: '/repo', - branch: 'main', - base: 'origin/main', - hasUncommittedChanges: false, - hasUpstream: true, - ahead: 0, - behind: 0 - }) - ).resolves.toMatchObject({ - canCreate: false, - blockedReason: 'default_branch', - defaultBaseRef: 'origin/main' - }) - }) - - it('blocks dirty tracked GitHub branches before PR creation', async () => { - await expect( - getHostedReviewCreationEligibility({ - repoPath: '/repo', - branch: 'feature/create-pr', - base: 'main', - hasUncommittedChanges: true, - hasUpstream: true, - ahead: 0, - behind: 0 - }) - ).resolves.toMatchObject({ - provider: 'github', - canCreate: false, - blockedReason: 'dirty', - nextAction: 'commit', - head: 'feature/create-pr' - }) - }) - - it('keeps dirty feature branches eligible for PR preparation when review lookup fails', async () => { - getHostedReviewForBranchMock.mockRejectedValueOnce(new Error('gh lookup failed')) - - await expect( - getHostedReviewCreationEligibility({ - repoPath: '/repo', - branch: 'feature/create-pr', - base: 'main', - hasUncommittedChanges: true, - hasUpstream: false, - ahead: 0, - behind: 0 - }) - ).resolves.toMatchObject({ - provider: 'github', - canCreate: false, - blockedReason: 'dirty', - nextAction: 'commit', - head: 'feature/create-pr' - }) - }) - - it('enables creation for clean, in-sync, authenticated GitHub feature branches', async () => { - await expect( - getHostedReviewCreationEligibility({ - repoPath: '/repo', - branch: 'refs/heads/feature/create-pr', - base: 'origin/main', - hasUncommittedChanges: false, - hasUpstream: true, - ahead: 0, - behind: 0 - }) - ).resolves.toMatchObject({ - provider: 'github', - canCreate: true, - blockedReason: null, - nextAction: null, - defaultBaseRef: 'origin/main', - head: 'feature/create-pr' - }) - }) - - it('detects a GitHub Enterprise Server branch as the GitHub provider (#8312)', async () => { - mockGitHubEnterpriseProvider() - - await expect( - getHostedReviewCreationEligibility({ - repoPath: '/repo', - branch: 'feature/create-pr', - base: 'origin/main', - hasUncommittedChanges: false, - hasUpstream: true, - ahead: 0, - behind: 0 - }) - ).resolves.toMatchObject({ - provider: 'github', - canCreate: true, - blockedReason: null, - nextAction: null - }) - - // Enterprise auth was already confirmed during detection; the gate must not - // fire a redundant gh probe. - expect(ghExecFileAsyncMock).not.toHaveBeenCalled() - }) - - it('resolves remote eligibility through SSH repo metadata without generating PR copy', async () => { - const remoteGit = { - exec: vi.fn(async () => ({ stdout: '', stderr: '' })) - } - getSshGitProviderMock.mockReturnValue(remoteGit) - - await expect( - getHostedReviewCreationEligibility({ - repoPath: '/remote/repo', - connectionId: 'ssh-1', - branch: 'feature/create-pr', - base: 'origin/main', - hasUncommittedChanges: false, - hasUpstream: true, - ahead: 0, - behind: 0 - }) - ).resolves.toMatchObject({ - provider: 'github', - canCreate: true, - head: 'feature/create-pr' - }) - - expect(getProjectSlugMock).toHaveBeenCalledWith('/remote/repo', 'ssh-1') - expect(getRepoSlugMock).toHaveBeenCalledWith('/remote/repo', 'ssh-1') - expect(getHostedReviewForBranchMock).toHaveBeenCalledWith( - expect.objectContaining({ repoPath: '/remote/repo', connectionId: 'ssh-1' }) - ) - expect(remoteGit.exec).not.toHaveBeenCalled() - }) - - it('offers push as the next action for authenticated branches with local-only commits', async () => { - await expect( - getHostedReviewCreationEligibility({ - repoPath: '/repo', - branch: 'feature/create-pr', - base: 'main', - hasUncommittedChanges: false, - hasUpstream: true, - ahead: 2, - behind: 0 - }) - ).resolves.toMatchObject({ - canCreate: false, - blockedReason: 'needs_push', - nextAction: 'push' - }) - }) - - it('enables creation for clean, in-sync, authenticated GitLab feature branches', async () => { - mockGitLabProvider() - - await expect( - getHostedReviewCreationEligibility({ - repoPath: '/repo', - branch: 'feature/gitlab', - base: 'main', - hasUncommittedChanges: false, - hasUpstream: true, - ahead: 0, - behind: 0 - }) - ).resolves.toMatchObject({ - provider: 'gitlab', - canCreate: true, - blockedReason: null, - nextAction: null, - head: 'feature/gitlab' - }) - expect(ghExecFileAsyncMock).not.toHaveBeenCalled() - expect(glabExecFileAsyncMock).toHaveBeenCalledWith( - ['auth', 'status', '--hostname', 'gitlab.com'], - { cwd: '/repo' } - ) - }) - - it('enables creation for clean, in-sync, token-configured Azure DevOps feature branches', async () => { - mockAzureDevOpsProvider() - - await expect( - getHostedReviewCreationEligibility({ - repoPath: '/repo', - branch: 'feature/azure', - base: 'main', - hasUncommittedChanges: false, - hasUpstream: true, - ahead: 0, - behind: 0 - }) - ).resolves.toMatchObject({ - provider: 'azure-devops', - canCreate: true, - blockedReason: null, - nextAction: null, - head: 'feature/azure' - }) - expect(isAzureDevOpsReviewCreationAuthenticatedMock).toHaveBeenCalledOnce() - expect(ghExecFileAsyncMock).not.toHaveBeenCalled() - expect(glabExecFileAsyncMock).not.toHaveBeenCalled() - }) - - it('enables creation for clean, in-sync, token-configured Gitea feature branches', async () => { - mockGiteaProvider() - - await expect( - getHostedReviewCreationEligibility({ - repoPath: '/repo', - branch: 'feature/gitea', - base: 'main', - hasUncommittedChanges: false, - hasUpstream: true, - ahead: 0, - behind: 0 - }) - ).resolves.toMatchObject({ - provider: 'gitea', - canCreate: true, - blockedReason: null, - nextAction: null, - head: 'feature/gitea' - }) - expect(isGiteaReviewCreationAuthenticatedMock).toHaveBeenCalledOnce() - expect(ghExecFileAsyncMock).not.toHaveBeenCalled() - expect(glabExecFileAsyncMock).not.toHaveBeenCalled() - }) -}) diff --git a/src/main/source-control/hosted-review-creation.ts b/src/main/source-control/hosted-review-creation.ts index 01caac804d8..1e19f1d4fa8 100644 --- a/src/main/source-control/hosted-review-creation.ts +++ b/src/main/source-control/hosted-review-creation.ts @@ -42,6 +42,10 @@ import { type HostedReviewCreationEligibilityInput = HostedReviewCreationEligibilityArgs & { connectionId?: string | null + // Why: only the create-time preflight enforces base-on-remote as a hard block; + // the renderer's eligibility probe passes the base as a candidate and relies on + // Change 1 to correct a local-only parent, so it must never set this. + enforceBaseOnRemote?: boolean } & HostedReviewExecutionOptions function stripRefPrefix(ref: string): string { @@ -133,6 +137,52 @@ async function getDefaultBaseRef( ) } +/** + * Whether the candidate base resolves to a remote-tracking branch on the + * executing host. + * + * Why: a stacked worktree's `worktree.baseRef` is typically a bare parent + * branch name with no remote qualifier, so the probe must match the branch + * under *any* configured remote rather than assume `origin` — otherwise fork + * workflows (`upstream/main`) would be missed. Runs through + * `runGitForHostedReview` so it evaluates on the same host that will run the + * provider create (native/WSL/SSH/relay). This reads the local remote-tracking + * snapshot, not the live remote; see the design doc's Open Questions for the + * ls-remote/staleness tradeoff left as a follow-up. + */ +async function baseRefExistsOnRemote( + candidate: string, + repoPath: string, + connectionId?: string | null, + options: HostedReviewExecutionOptions = {} +): Promise { + const base = normalizeHostedReviewBaseRef(candidate).trim() + if (!base) { + return false + } + const run = (argv: string[]): Promise<{ stdout: string }> => + runGitForHostedReview(repoPath, argv, connectionId, options) + + const patterns = [`refs/remotes/*/${base}`] + // Non-origin remote-qualified candidate (e.g. `fork/main`): the wildcard glob + // above only matches a branch literally named that because `*` does not cross `/`. + // Include the exact tracking ref directly. + if (base.includes('/')) { + patterns.push(`refs/remotes/${base}`) + } + + try { + // for-each-ref exits 0 whether or not the pattern matches, so a clean empty + // result is an authoritative "absent" while a thrown error is a transport + // failure. Never conflate the two: on an unreachable host, preserve the + // candidate rather than silently demoting a legitimately-pushed parent base. + const { stdout } = await run(['for-each-ref', '--count=1', '--format=%(refname)', ...patterns]) + return stdout.trim().length > 0 + } catch { + return true + } +} + async function getCurrentBranch( repoPath: string, connectionId?: string | null, @@ -255,9 +305,11 @@ async function isProviderAuthenticated( function blockedCreateResultForReason( reason: NonNullable, - provider: HostedReviewProvider + provider: HostedReviewProvider, + submittedBase?: string | null ): CreateHostedReviewResult | null { const copy = reviewCopy(provider) + const baseLabel = submittedBase?.trim() ? `"${submittedBase.trim()}" ` : '' const blockedCreateResultByReason = { auth_required: { ok: false, @@ -303,6 +355,11 @@ function blockedCreateResultForReason( ok: false, code: 'validation', error: `Create ${copy.shortLabel} failed: refresh source control status and try again.` + }, + base_not_on_remote: { + ok: false, + code: 'validation', + error: `Create ${copy.shortLabel} failed: the base branch ${baseLabel}hasn't been pushed to the remote. Choose a pushed base or push it first.` } } satisfies Partial< Record, CreateHostedReviewResult> @@ -311,7 +368,8 @@ function blockedCreateResultForReason( } function blockedEligibilityToCreateResult( - eligibility: HostedReviewCreationEligibility + eligibility: HostedReviewCreationEligibility, + submittedBase?: string | null ): CreateHostedReviewResult | null { if (eligibility.canCreate) { return null @@ -326,7 +384,11 @@ function blockedEligibilityToCreateResult( } } if (eligibility.blockedReason) { - return blockedCreateResultForReason(eligibility.blockedReason, eligibility.provider) + return blockedCreateResultForReason( + eligibility.blockedReason, + eligibility.provider, + submittedBase + ) } const copy = reviewCopy(eligibility.provider) return { @@ -358,20 +420,24 @@ async function validateCurrentBranchCanCreateReview( hasUncommittedChanges(repoPath, connectionId, options), getHostedReviewUpstreamStatus(repoPath, connectionId, options) ]) + const submittedBase = normalizeHostedReviewBaseRef(input.base) const eligibility = await getHostedReviewCreationEligibility({ repoPath, branch: requestedHead || currentBranch, - base: normalizeHostedReviewBaseRef(input.base), + base: submittedBase, hasUncommittedChanges: dirty, hasUpstream: upstreamStatus.hasUpstream, ahead: upstreamStatus.ahead, behind: upstreamStatus.behind, connectionId, + // Why: this is the last gate before the provider create, which targets the + // submitted base verbatim — enforce that the base exists on the remote here. + enforceBaseOnRemote: true, ...options }) // Why: renderer eligibility can be stale by submit time; the main process // is the last chance to avoid creating a PR from an out-of-date remote head. - return blockedEligibilityToCreateResult(eligibility) + return blockedEligibilityToCreateResult(eligibility, submittedBase) } catch (error) { console.warn('Hosted review creation preflight failed:', error) return { @@ -391,8 +457,22 @@ export async function getHostedReviewCreationEligibility( connectionId: args.connectionId, ...hostedReviewExecutionContext(args) }) - const defaultBaseRef = - args.base?.trim() || (await getDefaultBaseRef(args.repoPath, args.connectionId, args)) + // Why: an incoming base is only a *candidate* for the default merge target. A + // stacked worktree's parent base resolves on the remote only when it was + // actually pushed; a local-only parent must fall back to the repo default so + // the PR targets a ref the remote can resolve. Never regress to "no base" — + // keep the candidate if the repo default itself is unavailable. + const candidateBase = args.base?.trim() || null + const candidateBaseOnRemote = + candidateBase != null && + (await baseRefExistsOnRemote(candidateBase, args.repoPath, args.connectionId, args)) + let defaultBaseRef: string | null + if (candidateBase && candidateBaseOnRemote) { + defaultBaseRef = candidateBase + } else { + const repoDefaultBaseRef = await getDefaultBaseRef(args.repoPath, args.connectionId, args) + defaultBaseRef = repoDefaultBaseRef ?? candidateBase + } const baseBranch = defaultBaseRef ? normalizeHostedReviewBaseRef(defaultBaseRef) : null let review: Awaited> = null try { @@ -481,6 +561,20 @@ export async function getHostedReviewCreationEligibility( if ((args.ahead ?? 0) > 0) { return { ...baseResult, canCreate: false, blockedReason: 'needs_push', nextAction: 'push' } } + // Why: at create-time, `gh pr create` (and the other providers) target the + // submitted base verbatim — Change 1 only corrects the *default*, not a stale + // renderer's submitted value. Block a local-only submitted base here so it + // fails with actionable copy instead of the provider's opaque error. Only the + // create-time preflight enforces this; the renderer's eligibility probe leaves + // enforceBaseOnRemote unset so a local-only parent is silently auto-corrected. + if (args.enforceBaseOnRemote && candidateBase && !candidateBaseOnRemote) { + return { + ...baseResult, + canCreate: false, + blockedReason: 'base_not_on_remote', + nextAction: null + } + } return { ...baseResult, canCreate: Boolean(baseBranch), blockedReason: null, nextAction: null } } diff --git a/src/renderer/src/components/right-sidebar/source-control-create-pr-intent-flow.test.ts b/src/renderer/src/components/right-sidebar/source-control-create-pr-intent-flow.test.ts index 74853dd4147..516c0711369 100644 --- a/src/renderer/src/components/right-sidebar/source-control-create-pr-intent-flow.test.ts +++ b/src/renderer/src/components/right-sidebar/source-control-create-pr-intent-flow.test.ts @@ -143,14 +143,18 @@ describe('source-control Create PR intent flow helpers', () => { ).toEqual(['safe.ts', 'new.ts']) }) - it('prefers the current compare base over stale eligibility defaults', () => { + it('prefers the remote-validated eligibility default so it cannot diverge from the composer', () => { + // Why: the intent flow's eligibility is recomputed from the same compare + // base right before creation, so its default already corrects a local-only + // stacked parent to the repo default. The one-click path must target that + // same base as the composer, not the raw (possibly unpushable) compare base. expect( resolveCreatePrIntentReviewBase({ - currentBaseRef: 'refs/remotes/origin/release', + currentBaseRef: 'stacked-parent', eligibilityDefaultBaseRef: 'refs/remotes/origin/main', composerBaseRef: 'main' }) - ).toBe('release') + ).toBe('main') expect( resolveCreatePrIntentReviewBase({ @@ -161,6 +165,19 @@ describe('source-control Create PR intent flow helpers', () => { ).toBe('develop') }) + it('falls back to the compare base when eligibility supplies no default', () => { + // Why: never blank the base. If the main process could not resolve a default + // (no candidate on remote and repo default unavailable), keep the user's + // compare base rather than dropping to an empty target. + expect( + resolveCreatePrIntentReviewBase({ + currentBaseRef: 'refs/remotes/origin/release', + eligibilityDefaultBaseRef: null, + composerBaseRef: 'main' + }) + ).toBe('release') + }) + it('resolves safe remote steps for publish, push, and patch-equivalent force-push', () => { expect( resolveCreatePrIntentRemoteStep({ diff --git a/src/renderer/src/components/right-sidebar/source-control-create-pr-intent-flow.ts b/src/renderer/src/components/right-sidebar/source-control-create-pr-intent-flow.ts index 7e183122ec5..566ee54e1d7 100644 --- a/src/renderer/src/components/right-sidebar/source-control-create-pr-intent-flow.ts +++ b/src/renderer/src/components/right-sidebar/source-control-create-pr-intent-flow.ts @@ -103,10 +103,15 @@ export function resolveCreatePrIntentReviewBase({ eligibilityDefaultBaseRef?: string | null composerBaseRef?: string | null }): string { - // Why: the compare-base picker is the user's latest target; eligibility can - // lag behind while Create PR intent is preparing the branch. + // Why: prefer the remote-validated eligibility default over the raw compare + // base. The intent flow auto-submits, and its eligibility is recomputed from + // this same compare base right before creation — so `eligibilityDefaultBaseRef` + // already keeps a pushed base verbatim and corrects a local-only stacked parent + // to the repo default. Using it keeps the one-click path consistent with the + // composer instead of submitting a base the remote cannot resolve. Fall back to + // the compare base only when eligibility supplied no default. return normalizeHostedReviewBaseRef( - currentBaseRef?.trim() || eligibilityDefaultBaseRef?.trim() || composerBaseRef?.trim() || '' + eligibilityDefaultBaseRef?.trim() || currentBaseRef?.trim() || composerBaseRef?.trim() || '' ) } diff --git a/src/renderer/src/components/right-sidebar/source-control-create-review-blocked-action.ts b/src/renderer/src/components/right-sidebar/source-control-create-review-blocked-action.ts index e59354da5c8..2475a7d293e 100644 --- a/src/renderer/src/components/right-sidebar/source-control-create-review-blocked-action.ts +++ b/src/renderer/src/components/right-sidebar/source-control-create-review-blocked-action.ts @@ -66,6 +66,9 @@ export function resolveBlockedCreateReviewNoticeMessage( case 'existing_review': case 'fork_head_unsupported': case 'unsupported_provider': + // Why: base_not_on_remote is a create-time hard failure surfaced as an error + // result, not an inline-actionable eligibility state, so it is non-clickable. + case 'base_not_on_remote': case null: return null } diff --git a/src/renderer/src/components/right-sidebar/source-control-dropdown-items.ts b/src/renderer/src/components/right-sidebar/source-control-dropdown-items.ts index 58bbfe9454e..62801dcfe31 100644 --- a/src/renderer/src/components/right-sidebar/source-control-dropdown-items.ts +++ b/src/renderer/src/components/right-sidebar/source-control-dropdown-items.ts @@ -513,6 +513,8 @@ export function resolveDropdownItems(inputs: DropdownActionInputs): DropdownEntr return `A ${createReviewCopy.reviewLabel} already exists` case 'fork_head_unsupported': return 'Fork head unsupported' + case 'base_not_on_remote': + return 'Base branch is not on the remote' case null: case undefined: return upstreamLoading ? 'Checking branch status…' : 'Branch is not ready' diff --git a/src/renderer/src/components/right-sidebar/source-control-primary-create-pr-intent-action.ts b/src/renderer/src/components/right-sidebar/source-control-primary-create-pr-intent-action.ts index ce4849a89f9..6ee7bce037f 100644 --- a/src/renderer/src/components/right-sidebar/source-control-primary-create-pr-intent-action.ts +++ b/src/renderer/src/components/right-sidebar/source-control-primary-create-pr-intent-action.ts @@ -180,6 +180,7 @@ export function resolveDisabledCreatePrHeaderAction( case 'existing_review': case 'fork_head_unsupported': case 'unsupported_provider': + case 'base_not_on_remote': case null: title = translate( 'auto.components.right.sidebar.source.control.primary.action.f0c6e2a581', @@ -311,11 +312,7 @@ export function resolveCreatePrHeaderAction(inputs: PrimaryActionInputs): Primar return createPrIntent } - // Why: blocked notices are only for states the preparation intent cannot - // safely resolve, such as auth/default-branch/unsafe sync blockers. - if (canClickBlockedCreateReviewReason(inputs.hostedReviewCreation?.blockedReason)) { - return resolveDisabledCreatePrHeaderAction(inputs) - } - + // Why: any remaining blocked state (including create-time-only base_not_on_remote) + // falls back to the disabled header action with its explanatory title. return resolveDisabledCreatePrHeaderAction(inputs) } diff --git a/src/renderer/src/components/right-sidebar/useCreatePullRequestDialogFields.test.ts b/src/renderer/src/components/right-sidebar/useCreatePullRequestDialogFields.test.ts index e23db872e1b..d09fc41cc32 100644 --- a/src/renderer/src/components/right-sidebar/useCreatePullRequestDialogFields.test.ts +++ b/src/renderer/src/components/right-sidebar/useCreatePullRequestDialogFields.test.ts @@ -155,14 +155,36 @@ describe('useCreatePullRequestDialogFields', () => { } }) - it('prefers the selected current base ref over stale eligibility defaults', async () => { + it('prefers the remote-validated eligibility default over a stacked local-only base', async () => { + // Why: for a stacked worktree the current base is the local-only parent + // branch, which the main process resolves to the repo default. The seeded + // field must follow the remote-validated eligibility default, not the parent. const harness = renderDialogFields({ eligibility: createEligibility({ defaultBaseRef: 'refs/remotes/origin/main' }), - currentBaseRef: 'refs/remotes/origin/release' + currentBaseRef: 'stacked-parent' }) try { await harness.rerender({ eligibility: createEligibility({ defaultBaseRef: 'refs/remotes/origin/main' }), + currentBaseRef: 'stacked-parent' + }) + + expect(harness.current().base).toBe('main') + } finally { + harness.unmount() + } + }) + + it('falls back to the current base ref when eligibility supplies no default', async () => { + // Why: when the main process cannot resolve a default (e.g. origin/HEAD + // unset and no probes match), keep the current base rather than blanking it. + const harness = renderDialogFields({ + eligibility: createEligibility({ defaultBaseRef: null }), + currentBaseRef: 'refs/remotes/origin/release' + }) + try { + await harness.rerender({ + eligibility: createEligibility({ defaultBaseRef: null }), currentBaseRef: 'refs/remotes/origin/release' }) @@ -212,7 +234,7 @@ describe('useCreatePullRequestDialogFields', () => { const seedRevisions = { ...harness.current().fieldRevisions } await harness.rerender({ - eligibility: createEligibility(), + eligibility: createEligibility({ defaultBaseRef: 'refs/remotes/origin/release' }), currentBaseRef: 'refs/remotes/origin/release' }) expect(harness.current().base).toBe('release') diff --git a/src/renderer/src/components/right-sidebar/useCreatePullRequestDialogFields.ts b/src/renderer/src/components/right-sidebar/useCreatePullRequestDialogFields.ts index a480689334e..08a3b311de6 100644 --- a/src/renderer/src/components/right-sidebar/useCreatePullRequestDialogFields.ts +++ b/src/renderer/src/components/right-sidebar/useCreatePullRequestDialogFields.ts @@ -91,7 +91,13 @@ function resolveCreateReviewDefaultBaseRef({ currentBaseRef?: string | null eligibilityDefaultBaseRef?: string | null }): string { - return stripBaseRef(currentBaseRef?.trim() || eligibilityDefaultBaseRef?.trim() || '') + // Why: prefer the remote-validated main-process default over the worktree's + // local parent base. For a stacked worktree whose parent is local-only, + // `currentBaseRef` is that unpushable parent; the eligibility default has + // already fallen back to a ref the remote can resolve. Fall back to + // `currentBaseRef` only when eligibility supplied no default. Manual + // `setUserBase` still wins via the base-resync suppression. + return stripBaseRef(eligibilityDefaultBaseRef?.trim() || currentBaseRef?.trim() || '') } export function normalizeCreateReviewBaseSearchResults( diff --git a/src/renderer/src/components/terminal-pane/agent-completion-coordinator.test.ts b/src/renderer/src/components/terminal-pane/agent-completion-coordinator.test.ts index 6a4c8b197cf..957e0a9bb1e 100644 --- a/src/renderer/src/components/terminal-pane/agent-completion-coordinator.test.ts +++ b/src/renderer/src/components/terminal-pane/agent-completion-coordinator.test.ts @@ -1832,14 +1832,29 @@ describe('agent completion coordinator', () => { const turn = { prompt: 'fix the bug', agentType: 'codex' as const } coordinator.observeHookStatus({ state: 'working', ...turn }) - coordinator.observeHookStatus({ state: 'waiting', ...turn, toolName: 'exec_command', toolInput: 'ls' }) + coordinator.observeHookStatus({ + state: 'waiting', + ...turn, + toolName: 'exec_command', + toolInput: 'ls' + }) // First pause auto-resolves before the window elapses. - coordinator.observeHookStatus({ state: 'working', ...turn, toolName: 'exec_command', toolInput: 'ls' }) + coordinator.observeHookStatus({ + state: 'working', + ...turn, + toolName: 'exec_command', + toolInput: 'ls' + }) vi.advanceTimersByTime(CODEX_ATTENTION_QUIET_MS) expect(dispatchAttention).not.toHaveBeenCalled() // A later, genuinely-distinct pause must re-arm the debounce and fire. - coordinator.observeHookStatus({ state: 'waiting', ...turn, toolName: 'apply_patch', toolInput: 'diff' }) + coordinator.observeHookStatus({ + state: 'waiting', + ...turn, + toolName: 'apply_patch', + toolInput: 'diff' + }) expect(dispatchAttention).not.toHaveBeenCalled() vi.advanceTimersByTime(CODEX_ATTENTION_QUIET_MS) expect(dispatchAttention).toHaveBeenCalledTimes(1) diff --git a/src/renderer/src/hooks/agent-hook-completion-notifications.test.ts b/src/renderer/src/hooks/agent-hook-completion-notifications.test.ts index 1801c78bace..228af0058eb 100644 --- a/src/renderer/src/hooks/agent-hook-completion-notifications.test.ts +++ b/src/renderer/src/hooks/agent-hook-completion-notifications.test.ts @@ -112,9 +112,8 @@ describe('agent hook completion notifications', () => { // Why: the Codex permission-pause tests share a working→pause→quiet-window // sequence; centralizing it keeps the debounce advance (issue #8387) in one spot. async function observeCodexPermissionPause(state: 'waiting' | 'blocked'): Promise { - const { observeAgentHookCompletionForNotification } = await import( - './agent-hook-completion-notifications' - ) + const { observeAgentHookCompletionForNotification } = + await import('./agent-hook-completion-notifications') observeAgentHookCompletionForNotification({ paneKey, worktreeId: 'wt-1', diff --git a/src/shared/hosted-review.ts b/src/shared/hosted-review.ts index 864baceba87..04094cf0774 100644 --- a/src/shared/hosted-review.ts +++ b/src/shared/hosted-review.ts @@ -104,6 +104,10 @@ export type HostedReviewCreationBlockedReason = | 'fork_head_unsupported' | 'unsupported_provider' | 'existing_review' + // Why: a stacked worktree's local-only parent base is unresolvable on the + // remote; blocked at create-time so the submit fails with actionable copy + // instead of the provider's opaque error. + | 'base_not_on_remote' | null export type HostedReviewCreationNextAction = From 81cc966ea3a23c41e72f98f8aa9793143f5fef49 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Tue, 14 Jul 2026 03:12:08 +0000 Subject: [PATCH 02/17] release: v1.4.139-rc.2 --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 1bf2bc74f30..e62fce386e9 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "orca", - "version": "1.4.139-rc.1", + "version": "1.4.139-rc.2", "description": "Next-gen IDE for parallel agentic development", "homepage": "https://github.com/stablyai/orca", "author": "stablyai", From b379610cbedd9cfe7750c5e9d2b0464334dc423e Mon Sep 17 00:00:00 2001 From: Jinwoo Hong <73622457+Jinwoo-H@users.noreply.github.com> Date: Mon, 13 Jul 2026 20:48:00 -0700 Subject: [PATCH 03/17] fix(browser): handle Cmd-click popups without adopted WebContents (#8659) Co-authored-by: Orca --- src/main/browser/popup-origin-bar-window.test.ts | 6 ++++++ src/main/browser/popup-origin-bar-window.ts | 4 +++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/src/main/browser/popup-origin-bar-window.test.ts b/src/main/browser/popup-origin-bar-window.test.ts index c0161dd94e8..3e816e74b07 100644 --- a/src/main/browser/popup-origin-bar-window.test.ts +++ b/src/main/browser/popup-origin-bar-window.test.ts @@ -53,6 +53,11 @@ const { fakeElectron } = vi.hoisted(() => { webContents: FakeWebContents setBounds = vi.fn() constructor(options: { webContents?: FakeWebContents; webPreferences?: unknown }) { + // Why: Electron rejects explicit undefined instead of treating it as + // omitted, which the popup fallback path depends on. + if (Object.hasOwn(options, 'webContents') && options.webContents === undefined) { + throw new TypeError('options.webContents must be a WebContents') + } this.options = options this.webContents = options.webContents ?? createFakeWebContents() FakeWebContentsView.instances.push(this) @@ -186,6 +191,7 @@ describe('openPopupWithOriginBar', () => { it('loads the target itself only when no pre-created contents were provided', () => { const popup = openPopupWithOriginBar({}, 'https://example.com/login') + expect(lastViews().content.options).not.toHaveProperty('webContents') expect(popup.contentWebContents.loadURL).toHaveBeenCalledWith('https://example.com/login') }) diff --git a/src/main/browser/popup-origin-bar-window.ts b/src/main/browser/popup-origin-bar-window.ts index 504c862a5b8..c9ac057c177 100644 --- a/src/main/browser/popup-origin-bar-window.ts +++ b/src/main/browser/popup-origin-bar-window.ts @@ -120,7 +120,9 @@ export function openPopupWithOriginBar( webPreferences: { contextIsolation: true, nodeIntegration: false, sandbox: true } }) const contentView = new WebContentsView({ - webContents: options.webContents, + // Why: Electron rejects an explicitly undefined webContents; omitting it + // lets WebContentsView create contents for Cmd/Ctrl-click popups. + ...(options.webContents === undefined ? {} : { webContents: options.webContents }), webPreferences: options.webPreferences }) window.contentView.addChildView(contentView) From 5a3e4b44f85b74bee12f328961967551fa2b873b Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Mon, 13 Jul 2026 21:10:01 -0700 Subject: [PATCH 04/17] fix(lint): cover active/inactive in the terminal tab activity switch (#8660) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The WorktreeStatus union feeding activityDotState widened, and the switch-exhaustiveness gate now fails on main itself (verify does not run on pushes to main, so the break landed silently and blocks every PR's CI). Cover the two glyph-less states explicitly — matching the function's documented intent — and drop the now-unreachable default. --- src/renderer/src/components/tab-bar/TerminalTabLeadingIcon.tsx | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/renderer/src/components/tab-bar/TerminalTabLeadingIcon.tsx b/src/renderer/src/components/tab-bar/TerminalTabLeadingIcon.tsx index 5c3b0c7dc24..72e8ed3f276 100644 --- a/src/renderer/src/components/tab-bar/TerminalTabLeadingIcon.tsx +++ b/src/renderer/src/components/tab-bar/TerminalTabLeadingIcon.tsx @@ -35,7 +35,8 @@ function activityDotState(status: TerminalTabActivityStatus): AgentDotState | nu return 'permission' case 'done': return 'done' - default: + case 'active': + case 'inactive': return null } } From 9875b5528a3d4164b1556e8b1daf457d8bf21b7a Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Mon, 13 Jul 2026 21:24:33 -0700 Subject: [PATCH 05/17] Improve native chat style (#8654) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Redesign native chat interactive UI to match assistant-turn styling - Restyle approval and question cards as bordered cards docked in the composer region instead of top-border strips, and widen the chat column (max-w-3xl → max-w-4xl) to match the rest of the app - Rework the question card into a numbered pick-list with an always- present free-text row and explicit Next/Skip/Send action, replacing the prior "Other…" toggle flow - Hide the composer while a question card is active since it supplies its own answer input, and hold the card open until the paced answer send settles instead of dismissing on click - Auto-repin the message list to the bottom during in-place streaming growth via a ResizeObserver on the content, not just the viewport - Widen question-answer pacing (800ms→1000ms step, 300ms→500ms buffer) for reliability on slower machines / higher-latency SSH sessions - Restyle tool-run/tool-line disclosure to a right-side hover chevron and add a formatted (pretty-printed) detail view for diff-less calls - Add "Skip" i18n string across all locales * Extend paste bridge and question-card fixes for native chat interactive - Route pane-level Paste to the question card's free-text input when the composer is unmounted, so Cmd/Ctrl+V keeps working while a question prompt is showing. - Fix a stale-timer bug where a new interactive prompt could inherit the previous card's dismiss/submitting state and swallow its first answer. - Only render tool-run detail when there's actually content to show, and mark selected question options with aria-pressed for accessibility. --- .../native-chat/NativeChatApprovalCard.tsx | 58 +++-- .../native-chat/NativeChatComposerField.tsx | 9 +- .../native-chat/NativeChatInteractiveCard.tsx | 53 +++- .../native-chat/NativeChatMessageList.tsx | 37 ++- .../native-chat/NativeChatQuestionCard.tsx | 243 +++++++++++------- .../native-chat/NativeChatToolRun.tsx | 83 ++++-- .../components/native-chat/NativeChatView.tsx | 71 ++--- .../native-chat-runtime-send.test.ts | 8 +- .../native-chat/native-chat-runtime-send.ts | 6 +- .../native-chat/native-chat-tool-summary.ts | 20 ++ .../use-native-chat-context-menu.tsx | 17 ++ .../use-native-chat-interactive-send.ts | 16 +- .../use-native-chat-paste-bridge.ts | 34 ++- src/renderer/src/i18n/locales/en.json | 3 +- src/renderer/src/i18n/locales/es.json | 3 +- src/renderer/src/i18n/locales/ja.json | 3 +- src/renderer/src/i18n/locales/ko.json | 3 +- src/renderer/src/i18n/locales/zh.json | 3 +- 18 files changed, 446 insertions(+), 224 deletions(-) diff --git a/src/renderer/src/components/native-chat/NativeChatApprovalCard.tsx b/src/renderer/src/components/native-chat/NativeChatApprovalCard.tsx index cbf041f62e3..9b394ada907 100644 --- a/src/renderer/src/components/native-chat/NativeChatApprovalCard.tsx +++ b/src/renderer/src/components/native-chat/NativeChatApprovalCard.tsx @@ -19,35 +19,37 @@ export function NativeChatApprovalCard({ onChoose }: NativeChatApprovalCardProps): React.JSX.Element { return ( -
-
-
- -
-

{approval.title}

- {approval.detail ? ( -

- {approval.detail} -

- ) : null} +
+
+
+
+ +
+

{approval.title}

+ {approval.detail ? ( +

+ {approval.detail} +

+ ) : null} +
+
+
+ {approval.options.map((opt, i) => ( + + ))}
-
-
- {approval.options.map((opt, i) => ( - - ))}
diff --git a/src/renderer/src/components/native-chat/NativeChatComposerField.tsx b/src/renderer/src/components/native-chat/NativeChatComposerField.tsx index d3149c8c4ca..5a7b2a90255 100644 --- a/src/renderer/src/components/native-chat/NativeChatComposerField.tsx +++ b/src/renderer/src/components/native-chat/NativeChatComposerField.tsx @@ -85,8 +85,9 @@ export function NativeChatComposerField({ }: NativeChatComposerFieldProps): React.JSX.Element { return (
-
-
+ {/* Extra bottom padding keeps the input box off the window rim. */} +
+
{autocomplete.mode === 'slash' && autocomplete.suggestions.length > 0 ? ( {imageAttachments.length > 0 ? ( diff --git a/src/renderer/src/components/native-chat/NativeChatInteractiveCard.tsx b/src/renderer/src/components/native-chat/NativeChatInteractiveCard.tsx index 39bbba9a75e..947d5f65c3e 100644 --- a/src/renderer/src/components/native-chat/NativeChatInteractiveCard.tsx +++ b/src/renderer/src/components/native-chat/NativeChatInteractiveCard.tsx @@ -1,4 +1,4 @@ -import { useEffect, useMemo, useState } from 'react' +import { useEffect, useMemo, useRef, useState } from 'react' import { useAppStore } from '../../store' import { parseInteractivePrompt } from './native-chat-interactive-prompt' import { nativeChatCardDismissKey } from './native-chat-dismiss-key' @@ -23,11 +23,19 @@ import type { NativeChatInteractiveSend } from './use-native-chat-interactive-se export function NativeChatInteractiveCard({ paneKey, send, - canSend + canSend, + onShowingQuestionChange, + answerInputRef }: { paneKey: string send: NativeChatInteractiveSend canSend: boolean + /** Reports whether a question card is on screen so the view can replace the + * composer with it (the card's free-text row is the answer input). */ + onShowingQuestionChange?: (showing: boolean) => void + /** Forwarded to the question card's free-text row so pane-level Paste keeps + * a target while the composer is unmounted. */ + answerInputRef?: React.RefObject }): React.JSX.Element | null { const interactivePrompt = useAppStore( (s) => s.agentStatusByPaneKey[paneKey]?.interactivePrompt ?? null @@ -43,15 +51,40 @@ export function NativeChatInteractiveCard({ ) const cardKey = useMemo(() => nativeChatCardDismissKey(card), [card]) const [dismissedKey, setDismissedKey] = useState(null) + // A question answer is a paced multi-step write (body→Enter per question); keep + // the card up until it settles instead of dismissing on the click, so it doesn't + // vanish mid-send. `submitting` also gates a second submit racing the first. + const dismissTimerRef = useRef | null>(null) + const submittingRef = useRef(false) + const clearDismissTimer = (): void => { + if (dismissTimerRef.current) { + clearTimeout(dismissTimerRef.current) + dismissTimerRef.current = null + } + submittingRef.current = false + } + // Keyed on cardKey (and unmount): a new prompt can replace the current one + // before the settle timer fires, and a stale `submitting` gate would swallow + // the new card's first answer. + useEffect(() => clearDismissTimer, [cardKey]) // Forget the dismissal once the prompt clears so a fresh prompt can show. const present = card != null useEffect(() => { if (!present) { setDismissedKey(null) + clearDismissTimer() } }, [present]) + // Tell the view when a question card is up so it can hide the composer (this + // card supplies its own input). Reset on unmount so the composer comes back. + const showingQuestion = card?.kind === 'question' && canSend && cardKey !== dismissedKey + useEffect(() => { + onShowingQuestionChange?.(showingQuestion) + return () => onShowingQuestionChange?.(false) + }, [showingQuestion, onShowingQuestionChange]) + if (!card || !canSend || cardKey === dismissedKey) { return null } @@ -60,11 +93,23 @@ export function NativeChatInteractiveCard({ { - setDismissedKey(cardKey) - sendAnswer(text) + if (submittingRef.current) { + return + } + submittingRef.current = true + const settleMs = sendAnswer(text) + // Hold the card until the paced write finishes, then mark it answered + // (which hides it and restores the composer). + dismissTimerRef.current = setTimeout(() => { + setDismissedKey(cardKey) + submittingRef.current = false + dismissTimerRef.current = null + }, settleMs) }} onCancel={() => { + clearDismissTimer() setDismissedKey(cardKey) cancel() }} diff --git a/src/renderer/src/components/native-chat/NativeChatMessageList.tsx b/src/renderer/src/components/native-chat/NativeChatMessageList.tsx index 313845c2030..71aee362083 100644 --- a/src/renderer/src/components/native-chat/NativeChatMessageList.tsx +++ b/src/renderer/src/components/native-chat/NativeChatMessageList.tsx @@ -167,7 +167,9 @@ function MessageRow({ // transcript caught up.) return (
-
+ {/* User turns get a distinct muted fill (not the card/canvas color) so + the prompt reads apart from the assistant's body copy. */} +
{markdown ? ( <> @@ -213,7 +215,7 @@ function MessageRow({ ) : null} @@ -251,6 +253,7 @@ export function NativeChatMessageList({ failedDeliveryMessageIds?: ReadonlySet }): React.JSX.Element { const scrollRef = useRef(null) + const contentRef = useRef(null) const [stuckToBottom, setStuckToBottom] = useState(true) const [showJump, setShowJump] = useState(false) @@ -312,6 +315,11 @@ export function NativeChatMessageList({ if (!container) { return } + // Detach synchronously (not just via the pending onScroll) so an in-place + // streaming growth can't re-pin to the bottom mid-flight and fight this + // deliberate scroll. The ref is what the resize observer reads. + stuckToBottomRef.current = false + setStuckToBottom(false) const delta = el.getBoundingClientRect().top - container.getBoundingClientRect().top container.scrollTo({ top: container.scrollTop + delta, behavior: 'smooth' }) }, []) @@ -334,17 +342,31 @@ export function NativeChatMessageList({ } }, [messages.length, isWorking, showTypingIndicator, scrollToBottom]) - // Keep the affordances in sync if the container resizes (e.g. composer mounts, - // viewport reflow) without a scroll event. + // Content growing without a message-count change (a streaming assistant turn + // extends its own message in place) never re-fires the layout effect above. + // Observe the container so those in-place growths still re-pin: stay glued to + // the bottom while stuck, otherwise just refresh the jump affordance. This is + // what removes most "Jump to latest" clicks during a live response. useEffect(() => { const el = scrollRef.current if (!el || typeof ResizeObserver === 'undefined') { return } - const observer = new ResizeObserver(handleScroll) + const observer = new ResizeObserver(() => { + if (stuckToBottomRef.current) { + scrollToBottom() + } else { + handleScroll() + } + }) + // Observe the growing content, not just the fixed-height viewport, so an + // in-place streaming growth is seen; also watch the viewport for reflows. observer.observe(el) + if (contentRef.current) { + observer.observe(contentRef.current) + } return () => observer.disconnect() - }, [handleScroll]) + }, [handleScroll, scrollToBottom]) return (
@@ -354,7 +376,8 @@ export function NativeChatMessageList({ className="scrollbar-sleek h-full overflow-y-auto px-3 pt-10 pb-4 sm:px-4" >
void /** Dismiss the prompt (sends Escape to the agent). */ onCancel: () => void + /** Exposes the free-text row so pane-level Paste can target it while the + * card replaces the composer. */ + answerInputRef?: RefObject } -// Synthetic option value for the "Other…" free-text row, kept out of the -// answer text and replaced by the typed value when selected. -const OTHER = '__other__' - /** - * Native renderer for an agent's AskUserQuestion prompt as a wizard: one - * question per step with tabs across the top (tap to jump, a check once - * answered), single- or multi-select option rows, an "Other…" row that reveals a - * free-text input, and a Next button that advances and becomes "Send answer" on - * the last step. Matches the desktop chat's neutral shadcn styling. + * Native renderer for an agent's AskUserQuestion prompt: a numbered pick-list + * (mobile/Claude-Code parity) with a header + close, a hover-highlighted row per + * option, and an always-present free-text row for a custom answer. Single-select + * commits on click; multi-select toggles and confirms via the trailing action. + * Multi-question prompts step through tabs across the top. Neutral shadcn tokens. */ export function NativeChatQuestionCard({ prompt, onAnswer, - onCancel + onCancel, + answerInputRef }: NativeChatQuestionCardProps): React.JSX.Element { const [index, setIndex] = useState(0) const [selections, setSelections] = useState(() => prompt.questions.map(() => [])) const [otherText, setOtherText] = useState(() => prompt.questions.map(() => '')) - const toggle = (qi: number, label: string, multi: boolean): void => { - setSelections((prev) => { - const next = prev.map((s) => [...s]) - const cur = next[qi] ?? [] - if (multi) { - next[qi] = cur.includes(label) ? cur.filter((l) => l !== label) : [...cur, label] - } else { - next[qi] = cur.includes(label) ? [] : [label] - } - return next - }) - } + const total = prompt.questions.length + const isLast = index === total - 1 + const q = prompt.questions[index]! const setOther = (qi: number, value: string): void => { setOtherText((prev) => { @@ -53,28 +44,19 @@ export function NativeChatQuestionCard({ }) } - // The resolved answer for a question: picked labels plus the typed "Other" - // value (which replaces the synthetic OTHER marker). - const answerFor = (qi: number): string => { - const picked = (selections[qi] ?? []).filter((l) => l !== OTHER) - const other = (selections[qi] ?? []).includes(OTHER) ? (otherText[qi] ?? '').trim() : '' - return [...picked, other].filter((p) => p.length > 0).join(', ') + // The resolved answer for a question: picked labels plus any typed free-text. + const answerFor = (qi: number, sel = selections, oth = otherText): string => { + const picked = sel[qi] ?? [] + const other = (oth[qi] ?? '').trim() + return [...picked, ...(other ? [other] : [])].join(', ') } - const total = prompt.questions.length - const isLast = index === total - 1 - const currentAnswered = useMemo( - () => answerFor(index).length > 0, - // eslint-disable-next-line react-hooks/exhaustive-deps - [selections, otherText, index] - ) + const currentAnswered = answerFor(index).length > 0 - const submit = (): void => { - // Build per-question label lists, substituting the typed Other value, then - // format to one line per answered question. + const submitAll = (sel: string[][], oth: string[]): void => { const resolved = prompt.questions.map((_, i) => { - const picked = (selections[i] ?? []).filter((l) => l !== OTHER) - const other = (selections[i] ?? []).includes(OTHER) ? (otherText[i] ?? '').trim() : '' + const picked = sel[i] ?? [] + const other = (oth[i] ?? '').trim() return [...picked, ...(other ? [other] : [])] }) const text = formatAskAnswer(prompt, resolved) @@ -83,22 +65,60 @@ export function NativeChatQuestionCard({ } } - const advance = (): void => { + // Advance to the next question, or submit on the last one — always from an + // explicit snapshot so a just-committed single-select pick isn't lost to the + // async setState. + const advanceOrSubmit = (sel: string[][], oth: string[]): void => { if (isLast) { - submit() + submitAll(sel, oth) } else { setIndex((i) => Math.min(i + 1, total - 1)) } } - const q = prompt.questions[index]! - const otherSelected = (selections[index] ?? []).includes(OTHER) + // Selecting only highlights the row; submitting is an explicit step via the + // trailing Send/Next button. (Auto-submitting on the first click dismissed the + // card before the user saw any feedback, which read as "nothing happened".) + const pickOption = (label: string): void => { + setSelections((prev) => { + const next = prev.map((s) => [...s]) + const cur = next[index] ?? [] + if (q.multiSelect) { + next[index] = cur.includes(label) ? cur.filter((l) => l !== label) : [...cur, label] + } else { + next[index] = cur.includes(label) ? [] : [label] + } + return next + }) + } + + // Trailing action (also fired by Enter). On any non-final question this just + // advances — "Next" when answered, "Skip" when not — so skipping one question + // never discards answers already given on the others. Only the final question + // submits; an explicit Skip click there with nothing answered anywhere + // dismisses, but a reflexive Enter in the empty field is a no-op so it can't + // throw away the whole prompt. + const confirm = (fromKeyboard = false): void => { + if (!isLast) { + advanceOrSubmit(selections, otherText) + return + } + const anyAnswered = prompt.questions.some((_, i) => answerFor(i).length > 0) + if (anyAnswered) { + submitAll(selections, otherText) + } else if (!fromKeyboard) { + onCancel() + } + } return ( -
-
+ // Part of the composer: docked in the bottom input region, matching the + // composer's width and padding, rendered as the "ask" dialog card directly + // above the text input. Its free-text row is the answer input. +
+
{total > 1 ? ( -
+
{prompt.questions.map((qq, i) => ( +
+ + {/* Scroll only kicks in on long option lists; the sleek scrollbar rides + the card's right edge instead of crowding the choices. */} +
+ {q.options.map((opt, i) => ( toggle(index, opt.label, q.multiSelect)} + onSelect={() => pickOption(opt.label)} /> ))} - toggle(index, OTHER, q.multiSelect)} - /> - {otherSelected ? ( -