From a91ab04b1953a67cbf4e01f2c92d4a3e77ee78c8 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Thu, 9 Jul 2026 02:07:18 -0700 Subject: [PATCH] Optimize and integrate mobile PR view within source control hub - Lazy-load heavy PR comments and descriptions (Phase 2) only when the PR tab is active, using fast metadata (Phase 1) for the branch chip. - Unmount the PR body when inactive to avoid unnecessary comment tree re-renders and preserve WebView resources during commit text editing. - Implement soft-refresh on HEAD advancement to keep the ready UI visible while re-fetching checks post-commit. - Display the "Aborting..." label only when a merge or rebase abort is actively in flight. - Memoize the git history list and skip branch identity RPCs when gating the dock icon. --- mobile/app/h/[hostId]/pr/[worktreeId].tsx | 4 +- .../app/h/[hostId]/session/[worktreeId].tsx | 36 +- mobile/mock-source-control-hub.html | 433 ++++++++++++++++-- .../pr-sidebar/MobilePrViewPanel.tsx | 192 +------- .../components/pr-sidebar/PRSidebarHeader.tsx | 41 +- .../pr-sidebar/mobile-pr-sidebar-styles.ts | 14 + mobile/src/session/SessionDockColumn.tsx | 60 +-- mobile/src/session/mobile-pr-sidebar-state.ts | 41 ++ .../session/use-mobile-pr-branch-context.ts | 19 +- .../use-mobile-pr-sidebar-controller.test.ts | 62 +++ .../use-mobile-pr-sidebar-controller.ts | 235 ++++++++-- .../source-control/MobileGitHistoryList.tsx | 8 +- .../MobileSourceControlBranchCard.tsx | 7 +- .../MobileSourceControlHeader.tsx | 25 +- .../MobileSourceControlPanel.tsx | 142 ++++-- ...bile-source-control-conflict-abort.test.ts | 28 ++ .../mobile-source-control-conflict-abort.ts | 18 + 17 files changed, 988 insertions(+), 377 deletions(-) create mode 100644 mobile/src/source-control/mobile-source-control-conflict-abort.test.ts create mode 100644 mobile/src/source-control/mobile-source-control-conflict-abort.ts diff --git a/mobile/app/h/[hostId]/pr/[worktreeId].tsx b/mobile/app/h/[hostId]/pr/[worktreeId].tsx index 6e17b41019b..f2cf699269e 100644 --- a/mobile/app/h/[hostId]/pr/[worktreeId].tsx +++ b/mobile/app/h/[hostId]/pr/[worktreeId].tsx @@ -3,8 +3,8 @@ import { firstParam } from '../../../../src/source-control/mobile-source-control // The Pull Request view is now a segment of the Source Control hub. This route // stays as a thin redirect so existing deep links land on the hub with the Pull -// Request segment selected. The wide-layout dock still renders MobilePrViewPanel -// directly (it does not route through here). +// Request segment selected. The wide-layout dock opens the same hub with +// initialTab="pr" (SessionDockColumn), not this route. export default function PrRedirect() { const params = useLocalSearchParams<{ hostId?: string | string[] diff --git a/mobile/app/h/[hostId]/session/[worktreeId].tsx b/mobile/app/h/[hostId]/session/[worktreeId].tsx index 95aa8d7384e..da2aae83fce 100644 --- a/mobile/app/h/[hostId]/session/[worktreeId].tsx +++ b/mobile/app/h/[hostId]/session/[worktreeId].tsx @@ -856,20 +856,16 @@ export default function SessionScreen() { setActivePanel(null) } }, [canDockPanel, activePanel]) - // Session-level PR context feeds the docked PR panel and gates the GitHub-only - // PR entry so GitLab/other providers do not open a GitHub RPC surface. - const { - branch: prBranch, - headSha: prHeadSha, - status: prStatus, - isGithubRepo: prIsGithubRepo, - repoLoaded: prRepoContextLoaded, - loaded: prContextLoaded - } = useMobilePrBranchContext({ - client, - connState, - worktreeId - }) + // Session-level GitHub remote probe gates the PR dock icon so non-GitHub + // providers do not open the hosted-review surface. Branch/head/status for the + // hub are loaded inside MobileSourceControlPanel — skip the unused identity RPCs. + const { isGithubRepo: prIsGithubRepo, repoLoaded: prRepoContextLoaded } = + useMobilePrBranchContext({ + client, + connState, + worktreeId, + includeBranchIdentity: false + }) useEffect(() => { if (prRepoContextLoaded && !prIsGithubRepo && activePanel === 'pr') { setActivePanel(null) @@ -4438,8 +4434,9 @@ export default function SessionScreen() { hostId, worktreeId, name: worktreeName || '', - // Source control's post-diff-open dismissal keys off origin: 'session' (U2). - ...(action.panel === 'sourceControl' ? { origin: 'session' } : {}), + // SC + PR both land on the source-control hub; post-diff-open dismissal + // keys off origin: 'session' (U2). Files keeps its own route without origin. + ...(action.panel === 'sourceControl' || action.panel === 'pr' ? { origin: 'session' } : {}), // The PR panel routes into the hub's Pull Request segment via descriptor params. ...descriptor.params } @@ -5127,13 +5124,6 @@ export default function SessionScreen() { hostId={hostId} worktreeId={worktreeId} name={worktreeName || ''} - client={client} - connState={connState} - branch={prBranch} - headSha={prHeadSha} - gitStatus={prStatus} - isGithubRepo={prIsGithubRepo} - branchContextLoaded={prContextLoaded && prRepoContextLoaded} availableWidth={sessionContentRowWidth} onRequestClose={() => setActivePanel(null)} onFileOpenStart={handleFileOpenStart} diff --git a/mobile/mock-source-control-hub.html b/mobile/mock-source-control-hub.html index 8b09552ac7a..0a64d75ecbf 100644 --- a/mobile/mock-source-control-hub.html +++ b/mobile/mock-source-control-hub.html @@ -1258,21 +1258,54 @@
- + + + M - + + + + EditorFileTab.tsx ‎src/renderer/src/components/tab-bar/ - +12−4 + +12−4 - + + +
@@ -1293,17 +1326,53 @@
+Stage
🗑Discard
- + + M - + + + CenterSplitDivider.tsx ‎src/renderer/src/components/split-view/ - +34−8 + +34−8 - + +
+Stage
🗑Discard
- + + M - + + + split-view-geometry.ts ‎src/renderer/src/lib/ - +6−2 + +6−2 - + +
+Stage
🗑Discard
- + + A - + + + SplitDividerHandle.test.tsx - ‎src/renderer/src/components/split-view/__tests__/ - +48−0 + ‎src/renderer/src/components/split-view/__tests__/ + +48−0 - + +
@@ -1356,22 +1499,78 @@
-
- + + + Commit & Push
- + + + + Commit & Sync
@@ -1383,7 +1582,13 @@ 0 selected - +
@@ -1396,10 +1601,14 @@
Open #7701 - octocat wants to merge into main + octocat wants to merge into main
Make center split divider more visible
-
make-center-split-more-visible → main · updated 20m ago
+
+ make-center-split-more-visible → main · updated 20m ago +
@@ -1407,25 +1616,71 @@ 1 check failing
- + + + e2e / playwright (ubuntu-latest) Failing · 1 test · 2m 14s - +
Merge method - Squash and merge - + Squash and merge + + +
@@ -1433,10 +1688,25 @@
Auto-merge when checks pass
Merges automatically once required checks are green
-
+
+ +
1 failing check must pass before merging
@@ -1448,20 +1718,37 @@ · 3
- R + R rauchg · 18m ago - Can we clamp the divider width so it doesn't overlap the gutter on narrow panes? + Can we clamp the divider width so it doesn't overlap the gutter on narrow + panes? CenterSplitDivider.tsx:42
- M + M maren · 25m ago Add a test for the min-width geometry branch. @@ -1474,12 +1761,12 @@ Reviewers
- O + O octocat Approved
- D + D dstndev Review requested
@@ -1492,12 +1779,41 @@
- - build / typecheckPassed · 1m 02s + + + build / typecheckPassed · 1m 02s
- - lint / oxlintPassed · 38s + + + lint / oxlintPassed · 38s
@@ -1507,18 +1823,26 @@
-
+
Make center split divider more visible
-
a1b2c3d · Jinjing · 18m ago
+
+ a1b2c3d · Jinjing · 18m ago +
Add split-view geometry helper
-
9f8e7d6 · Jinjing · 42m ago
+
+ 9f8e7d6 · Jinjing · 42m ago +
@@ -1527,11 +1851,17 @@
-
+
Merge pull request #7699 from orca/main
-
0f7658d · web-flow · 3h ago
+
+ 0f7658d · web-flow · 3h ago +
@@ -1645,10 +1975,13 @@ diff: '', file: '', stage: '', - discard: '', + discard: + '', copy: '', - remote: '', - merge: '', + remote: + '', + merge: + '', hash: '', message: '', sparkle: '' diff --git a/mobile/src/components/pr-sidebar/MobilePrViewPanel.tsx b/mobile/src/components/pr-sidebar/MobilePrViewPanel.tsx index 91cac6f2fe8..5eb17e243d9 100644 --- a/mobile/src/components/pr-sidebar/MobilePrViewPanel.tsx +++ b/mobile/src/components/pr-sidebar/MobilePrViewPanel.tsx @@ -1,20 +1,13 @@ -import { useEffect } from 'react' -import { Pressable, StyleSheet, Text, View } from 'react-native' -import { SafeAreaView, useSafeAreaInsets } from 'react-native-safe-area-context' -import { useRouter } from 'expo-router' -import { ChevronLeft, ExternalLink, X } from 'lucide-react-native' -import { colors, radii, spacing, typography } from '../../theme/mobile-theme' +import { StyleSheet, View } from 'react-native' +import { useSafeAreaInsets } from 'react-native-safe-area-context' +import { colors } from '../../theme/mobile-theme' import type { ConnectionState } from '../../transport/types' import type { RpcClient } from '../../transport/rpc-client' import type { MobileGitStatusResult } from '../../source-control/mobile-git-status' -import { - useMobilePrSidebarController, - type MobilePrSidebarController -} from '../../session/use-mobile-pr-sidebar-controller' +import type { MobilePrSidebarController } from '../../session/use-mobile-pr-sidebar-controller' import { MobilePRSidebar } from '../MobilePRSidebar' -import { openMobilePrUrl } from '../MobilePrComposeSheet' -type BodyProps = { +type Props = { client: RpcClient | null connState: ConnectionState worktreeId: string @@ -24,32 +17,11 @@ type BodyProps = { isGithubRepo?: boolean branchContextLoaded?: boolean controller: MobilePrSidebarController - // 'chromeless' drops the panel's own header + SafeAreaView so it can live inside - // the source-control hub, which owns the header, segmented control, and chrome. - chromeless?: boolean - // When false, the parent (the hub) owns triggering the initial load — the body - // must not also fire it, or a ?tab=pr deep link double-fetches. Default true so - // the standalone route stays self-loading. - autoLoad?: boolean - embedded?: boolean - onRequestClose?: () => void -} - -type Props = Omit - -// Standalone entry (the /pr route + wide-layout dock): owns its controller and -// self-loads. The hub uses MobilePrViewPanelBody with a shared controller instead. -export function MobilePrViewPanel(props: Props) { - const controller = useMobilePrSidebarController({ - client: props.client, - connState: props.connState, - worktreeId: props.worktreeId, - branch: props.branch, - headSha: props.headSha - }) - return } +// Chromeless PR sidebar body for the source-control hub's Pull Request segment. +// The hub owns the header, segmented control, load triggers, and the shared +// controller (one fetch feeds both the branch-card chip and this body). export function MobilePrViewPanelBody({ client, connState, @@ -59,28 +31,10 @@ export function MobilePrViewPanelBody({ gitStatus, isGithubRepo = true, branchContextLoaded = true, - controller, - chromeless = false, - autoLoad = true, - embedded = false, - onRequestClose -}: BodyProps) { - const router = useRouter() + controller +}: Props) { const insets = useSafeAreaInsets() - // A docked/full-screen PR panel is always visible — there is no drawer to open, - // so trigger the load directly once context is ready rather than gating on the - // showPRSidebar overlay flag (KTD4). Skipped when the hub owns the trigger. - const prSidebarKind = controller.prSidebarState.kind - const refetch = controller.refetchPRSidebar - useEffect(() => { - if (autoLoad && branch && isGithubRepo && prSidebarKind === 'hidden') { - refetch() - } - }, [autoLoad, branch, isGithubRepo, prSidebarKind, refetch]) - - // Embedded: the dock column applies the bottom inset; full-screen relies on its own - // SafeAreaView (edges top only), so content must clear the home indicator itself. const sidebarState = !branchContextLoaded ? ({ kind: 'loading' } as const) : !isGithubRepo @@ -94,91 +48,22 @@ export function MobilePrViewPanelBody({ message: 'Current branch unavailable.' } as const) : controller.prSidebarState - // Why: open-on-host lives in the chrome so the PR URL is always flush-right of - // the screen title, even when the body is scrolled past the PR header. - const prUrl = - sidebarState.kind === 'ready' && sidebarState.data.pr.url ? sidebarState.data.pr.url : null - const prNumber = sidebarState.kind === 'ready' ? sidebarState.data.pr.number : null - const openPr = prUrl ? () => openMobilePrUrl(prUrl) : undefined - const openPrControl = openPr ? ( - [styles.iconButton, pressed && styles.iconButtonPressed]} - onPress={openPr} - hitSlop={8} - accessibilityRole="link" - accessibilityLabel={ - prNumber != null - ? `Open pull request #${prNumber} on the web` - : 'Open pull request on the web' - } - > - - - ) : null - const sidebar = ( - - ) - - // Chromeless: the hub owns the header + chrome, so render just the scrollable body. - if (chromeless) { - return {sidebar} - } - - if (embedded) { - return ( - - - - [styles.iconButton, pressed && styles.iconButtonPressed]} - onPress={onRequestClose} - hitSlop={8} - accessibilityLabel="Close pull request panel" - > - - - - Pull Request - - {openPrControl} - - - {sidebar} - - ) - } return ( - - - - [styles.iconButton, pressed && styles.iconButtonPressed]} - onPress={() => router.back()} - hitSlop={8} - accessibilityLabel="Back to session" - > - - - - Pull Request - - {openPrControl} - - - {sidebar} - + + + ) } @@ -186,34 +71,5 @@ const styles = StyleSheet.create({ container: { flex: 1, backgroundColor: colors.bgBase - }, - header: { - backgroundColor: colors.bgPanel, - borderBottomWidth: StyleSheet.hairlineWidth, - borderBottomColor: colors.borderSubtle - }, - topBar: { - minHeight: 58, - flexDirection: 'row', - alignItems: 'center', - gap: spacing.md, - paddingHorizontal: spacing.md - }, - iconButton: { - width: 36, - height: 36, - alignItems: 'center', - justifyContent: 'center', - borderRadius: radii.button - }, - iconButtonPressed: { - backgroundColor: colors.bgRaised - }, - title: { - flex: 1, - minWidth: 0, - color: colors.textPrimary, - fontSize: typography.titleSize, - fontWeight: '600' } }) diff --git a/mobile/src/components/pr-sidebar/PRSidebarHeader.tsx b/mobile/src/components/pr-sidebar/PRSidebarHeader.tsx index d974089c326..3d342820bbf 100644 --- a/mobile/src/components/pr-sidebar/PRSidebarHeader.tsx +++ b/mobile/src/components/pr-sidebar/PRSidebarHeader.tsx @@ -1,6 +1,6 @@ import { useState } from 'react' import { ActivityIndicator, Pressable, Text, TextInput, View } from 'react-native' -import { ArrowRight, Pencil } from 'lucide-react-native' +import { ArrowRight, ExternalLink, Pencil } from 'lucide-react-native' import { colors } from '../../theme/mobile-theme' import type { GitHubWorkItemDetails, PRInfo } from '../../../../src/shared/types' import type { MobilePrTitleAction } from '../../session/use-mobile-pr-title-action' @@ -36,19 +36,32 @@ export function PRSidebarHeader({ pr, details, titleAction }: Props) { return ( - [ - styles.badge, - { borderColor: badgeColor }, - pressed && { opacity: 0.6 } - ]} - > - {badge.label} - + + [ + styles.badge, + { borderColor: badgeColor }, + pressed && { opacity: 0.6 } + ]} + > + {badge.label} + + {openPr ? ( + [styles.openWebButton, pressed && { opacity: 0.6 }]} + > + + + ) : null} + hostId: string worktreeId: string name: string - client: RpcClient | null - connState: ConnectionState - branch: string | null - headSha: string | null - gitStatus: MobileGitStatusResult | null - isGithubRepo: boolean - branchContextLoaded: boolean availableWidth: number onRequestClose: () => void onFileOpenStart?: () => void @@ -40,13 +29,6 @@ export function SessionDockColumn({ hostId, worktreeId, name, - client, - connState, - branch, - headSha, - gitStatus, - isGithubRepo, - branchContextLoaded, availableWidth, onRequestClose, onFileOpenStart, @@ -63,13 +45,6 @@ export function SessionDockColumn({ hostId={hostId} worktreeId={worktreeId} name={name} - client={client} - connState={connState} - branch={branch} - headSha={headSha} - gitStatus={gitStatus} - isGithubRepo={isGithubRepo} - branchContextLoaded={branchContextLoaded} onRequestClose={onRequestClose} onFileOpenStart={onFileOpenStart} onOpenedFileDiff={onOpenedFileDiff} @@ -85,18 +60,14 @@ const DockPanelContent = memo(function DockPanelContent({ hostId, worktreeId, name, - client, - connState, - branch, - headSha, - gitStatus, - isGithubRepo, - branchContextLoaded, onRequestClose, onFileOpenStart, onOpenedFileDiff }: DockPanelContentProps) { - if (activePanel === 'sourceControl') { + // Source Control and Pull Request share one hub instance so swapping dock icons + // keeps commit draft / visited-tab / scroll state and lands on the right segment + // (design: PR dock maps to hub + tab=pr). + if (activePanel === 'sourceControl' || activePanel === 'pr') { return ( ) } - if (activePanel === 'files') { - return ( - - ) - } return ( - diff --git a/mobile/src/session/mobile-pr-sidebar-state.ts b/mobile/src/session/mobile-pr-sidebar-state.ts index 5f5b679b210..4ce32a7cec7 100644 --- a/mobile/src/session/mobile-pr-sidebar-state.ts +++ b/mobile/src/session/mobile-pr-sidebar-state.ts @@ -140,6 +140,47 @@ export async function loadPrSidebarDetails( } } +// UI treats details===null as "still loading". After phase 2 finishes (success or +// non-fatal failure), never leave null — use prior comments if any, else empty body. +export function resolvePrSidebarDetailsAfterPhase2(args: { + fetched: GitHubWorkItemDetails | null + prior: GitHubWorkItemDetails | null + pr: PRInfo +}): GitHubWorkItemDetails { + if (args.fetched != null) { + return args.fetched + } + if (args.prior != null) { + return args.prior + } + return emptyPrSidebarDetails(args.pr) +} + +// Placeholder details so Description/Comments leave the spinner after a failed phase 2. +export function emptyPrSidebarDetails(pr: PRInfo): GitHubWorkItemDetails { + return { + item: { + id: `pr-${pr.number}`, + type: 'pr', + number: pr.number, + title: pr.title, + state: pr.state, + url: pr.url, + labels: [], + updatedAt: pr.updatedAt, + author: null + }, + body: '', + comments: [] + } +} + +// Soft head refresh may restart an in-flight phase-1 load; only skip when there is +// no visible/in-flight sidebar work (hidden or terminal error states). +export function shouldSoftRefreshPrSidebarOnHeadChange(kind: PrSidebarState['kind']): boolean { + return kind === 'ready' || kind === 'none' || kind === 'loading' +} + // Stale-response guard (KTD6): a load tagged with an older sequence must not // overwrite a newer one. The hook bumps a monotonic counter per load. export function shouldApplyResult(resultSeq: number, latestSeq: number): boolean { diff --git a/mobile/src/session/use-mobile-pr-branch-context.ts b/mobile/src/session/use-mobile-pr-branch-context.ts index ef2cf1a48d5..d0816b02f71 100644 --- a/mobile/src/session/use-mobile-pr-branch-context.ts +++ b/mobile/src/session/use-mobile-pr-branch-context.ts @@ -34,12 +34,16 @@ export function deriveMobilePrBranchContext( // Loads repo eligibility independently from branch/SHA. The header PR icon only // needs the cheap GitHub probe; the panel can keep loading branch context after // the entry point is already stable in the top bar. +// +// `includeBranchIdentity: false` skips git.status + branchCompare — use when the +// caller only gates a PR entry on hosted-repo eligibility (session dock icon). export function useMobilePrBranchContext(input: { client: RpcClient | null connState: ConnectionState worktreeId: string + includeBranchIdentity?: boolean }): MobilePrBranchContext { - const { client, connState, worktreeId } = input + const { client, connState, worktreeId, includeBranchIdentity = true } = input const [context, setContext] = useState({ branch: null, headSha: null, @@ -60,7 +64,7 @@ export function useMobilePrBranchContext(input: { status: null, isGithubRepo: false, repoLoaded: false, - loaded: false + loaded: !includeBranchIdentity }) return } @@ -70,7 +74,8 @@ export function useMobilePrBranchContext(input: { status: null, isGithubRepo: false, repoLoaded: false, - loaded: false + // Repo-only mode is "loaded" for branch fields immediately (they stay null). + loaded: !includeBranchIdentity }) void loadMobilePrRepoContext(client, worktreeId) @@ -95,6 +100,12 @@ export function useMobilePrBranchContext(input: { } }) + if (!includeBranchIdentity) { + return () => { + cancelled = true + } + } + void loadMobilePrBranchIdentity(client, worktreeId) .then((next) => { if (!cancelled) { @@ -121,7 +132,7 @@ export function useMobilePrBranchContext(input: { return () => { cancelled = true } - }, [ready, client, worktreeId]) + }, [ready, client, worktreeId, includeBranchIdentity]) return context } diff --git a/mobile/src/session/use-mobile-pr-sidebar-controller.test.ts b/mobile/src/session/use-mobile-pr-sidebar-controller.test.ts index 005c4dc0e5b..de77433fcb6 100644 --- a/mobile/src/session/use-mobile-pr-sidebar-controller.test.ts +++ b/mobile/src/session/use-mobile-pr-sidebar-controller.test.ts @@ -4,11 +4,15 @@ import type { HostedReviewInfo } from '../../../src/shared/hosted-review' import type { GitHubPrReadOutcome } from './github-pr-rpc' import { classifyPrSidebarFailure, + emptyPrSidebarDetails, loadPrSidebarData, loadPrSidebarDetails, + resolvePrSidebarDetailsAfterPhase2, shouldApplyResult, + shouldSoftRefreshPrSidebarOnHeadChange, type PrSidebarLoadDeps } from './mobile-pr-sidebar-state' +import { buildMobilePrSidebarIdentity } from './use-mobile-pr-sidebar-controller' function ok(result: T): GitHubPrReadOutcome { return { ok: true, result } @@ -200,3 +204,61 @@ describe('shouldApplyResult', () => { expect(shouldApplyResult(2, 3)).toBe(false) }) }) + +describe('shouldSoftRefreshPrSidebarOnHeadChange', () => { + it('restarts ready/none/loading so mid-flight head advances are not stuck', () => { + expect(shouldSoftRefreshPrSidebarOnHeadChange('ready')).toBe(true) + expect(shouldSoftRefreshPrSidebarOnHeadChange('none')).toBe(true) + expect(shouldSoftRefreshPrSidebarOnHeadChange('loading')).toBe(true) + }) + + it('skips hidden and terminal failures (chip bootstrap / retry own those)', () => { + expect(shouldSoftRefreshPrSidebarOnHeadChange('hidden')).toBe(false) + expect(shouldSoftRefreshPrSidebarOnHeadChange('error')).toBe(false) + expect(shouldSoftRefreshPrSidebarOnHeadChange('blocked')).toBe(false) + }) +}) + +describe('resolvePrSidebarDetailsAfterPhase2', () => { + it('prefers a successful fetch over prior details', () => { + const prior = emptyPrSidebarDetails(PR) + expect(resolvePrSidebarDetailsAfterPhase2({ fetched: DETAILS, prior, pr: PR })).toBe(DETAILS) + }) + + it('keeps prior details when the fetch is non-fatal null', () => { + const prior = emptyPrSidebarDetails(PR) + expect(resolvePrSidebarDetailsAfterPhase2({ fetched: null, prior, pr: PR })).toBe(prior) + }) + + it('uses empty details when phase 2 fails with nothing prior (no forever spinner)', () => { + const empty = resolvePrSidebarDetailsAfterPhase2({ + fetched: null, + prior: null, + pr: PR + }) + expect(empty.body).toBe('') + expect(empty.comments).toEqual([]) + expect(empty.item.number).toBe(7) + expect(empty.item.type).toBe('pr') + }) +}) + +describe('buildMobilePrSidebarIdentity', () => { + it('keys identity on worktree + branch, not head SHA', () => { + expect(buildMobilePrSidebarIdentity({ worktreeId: 'w1', branch: 'feat' })).toBe('w1\u0000feat') + // Head advances on commit must not form a new identity — soft refresh keeps ready UI. + expect(buildMobilePrSidebarIdentity({ worktreeId: 'w1', branch: 'feat' })).toBe( + buildMobilePrSidebarIdentity({ worktreeId: 'w1', branch: 'feat' }) + ) + }) + + it('is null without a branch and changes when branch or worktree changes', () => { + expect(buildMobilePrSidebarIdentity({ worktreeId: 'w1', branch: null })).toBeNull() + expect(buildMobilePrSidebarIdentity({ worktreeId: 'w1', branch: 'a' })).not.toBe( + buildMobilePrSidebarIdentity({ worktreeId: 'w1', branch: 'b' }) + ) + expect(buildMobilePrSidebarIdentity({ worktreeId: 'w1', branch: 'a' })).not.toBe( + buildMobilePrSidebarIdentity({ worktreeId: 'w2', branch: 'a' }) + ) + }) +}) diff --git a/mobile/src/session/use-mobile-pr-sidebar-controller.ts b/mobile/src/session/use-mobile-pr-sidebar-controller.ts index ea3da7181d8..f59242a0583 100644 --- a/mobile/src/session/use-mobile-pr-sidebar-controller.ts +++ b/mobile/src/session/use-mobile-pr-sidebar-controller.ts @@ -11,7 +11,9 @@ import { import { loadPrSidebarData, loadPrSidebarDetails, + resolvePrSidebarDetailsAfterPhase2, shouldApplyResult, + shouldSoftRefreshPrSidebarOnHeadChange, type PrSidebarLoadDeps, type PrSidebarState } from './mobile-pr-sidebar-state' @@ -27,12 +29,20 @@ type PrSidebarControllerInput = { headSha: string | null } -function buildPrSidebarIdentity(args: { +// Load options for the shared PR controller. The Source Control hub chip only needs +// phase 1 (PR + checks); phase 2 (comments/body) is heavy and should wait until the +// Pull Request segment is actually open. +export type PrSidebarLoadOptions = { + includeDetails?: boolean +} + +// Identity is worktree + branch only. Head SHA advances on every commit and must not +// wipe a ready chip/sidebar to "loading" — soft-refresh uses the new head instead. +export function buildMobilePrSidebarIdentity(args: { worktreeId: string branch: string | null - headSha: string | null }): string | null { - return args.branch ? `${args.worktreeId}\u0000${args.branch}\u0000${args.headSha ?? ''}` : null + return args.branch ? `${args.worktreeId}\u0000${args.branch}` : null } export function useMobilePrSidebarController(input: PrSidebarControllerInput) { @@ -41,13 +51,27 @@ export function useMobilePrSidebarController(input: PrSidebarControllerInput) { // independent of whether the branch has an open PR (a no-PR branch shows an // empty state rather than hiding the icon). const [isGithubRepo, setIsGithubRepo] = useState(false) + // False until the probe resolves for this worktree. Consumers gate "unavailable + // for this provider" copy on it — isGithubRepo=false is meaningless mid-probe. + const [repoProbeLoaded, setRepoProbeLoaded] = useState(false) const [state, setState] = useState({ kind: 'hidden' }) const [showPRSidebar, setShowPRSidebar] = useState(false) const loadSeqRef = useRef(0) + // Phase-2-only fetches use a separate sequence so they cannot cancel a concurrent + // phase-1 soft refresh (and vice versa) when chip bootstrap left details null. + const detailsSeqRef = useRef(0) + // The (seq, prNumber) of the phase-2 fetch currently in flight. The hub's + // fill-in effect fires as soon as phase 1 renders ready with null details — + // exactly when load()'s own phase 2 just started. Without this claim, every + // cold PR-segment open fetched the heavy details payload twice. + const detailsInFlightRef = useRef<{ seq: number; prNumber: number } | null>(null) const stateIdentityRef = useRef(null) + const stateRef = useRef(state) + stateRef.current = state + const headShaRef = useRef(headSha) const ready = client !== null && connState === 'connected' && !!branch - const identity = buildPrSidebarIdentity({ worktreeId, branch, headSha }) + const identity = buildMobilePrSidebarIdentity({ worktreeId, branch }) const buildDeps = useCallback((): PrSidebarLoadDeps | null => { if (!client) { @@ -63,15 +87,22 @@ export function useMobilePrSidebarController(input: PrSidebarControllerInput) { }, [client]) // Probe whether this is a GitHub repo to decide icon availability (GitHub-only). + // Worktree change must reset eligibility; a brief disconnect must not — otherwise + // the hub shows "unavailable for this provider" and hides the chip mid-session. + useEffect(() => { + setIsGithubRepo(false) + setRepoProbeLoaded(false) + }, [worktreeId]) + useEffect(() => { let cancelled = false if (!ready || !client) { - setIsGithubRepo(false) return } void fetchGithubRepoSlug(client, worktreeId).then((outcome) => { if (!cancelled) { setIsGithubRepo(outcome.ok && outcome.result !== null) + setRepoProbeLoaded(true) } }) return () => { @@ -82,47 +113,183 @@ export function useMobilePrSidebarController(input: PrSidebarControllerInput) { useEffect(() => { if (!identity) { loadSeqRef.current += 1 + detailsSeqRef.current += 1 stateIdentityRef.current = null setState({ kind: 'hidden' }) return } if (stateIdentityRef.current !== null && stateIdentityRef.current !== identity) { - // Why: ready/loading data is scoped to branch+head. A branch switch must - // not let the open panel keep rendering the previous PR as "fresh." + // Why: ready/loading data is scoped to branch. A branch switch must not let + // the open panel keep rendering the previous PR as "fresh." loadSeqRef.current += 1 + detailsSeqRef.current += 1 stateIdentityRef.current = null setState({ kind: 'hidden' }) } }, [identity]) - const load = useCallback(async () => { + const load = useCallback( + async (options?: PrSidebarLoadOptions) => { + const includeDetails = options?.includeDetails ?? true + const deps = buildDeps() + const loadIdentity = identity + if (!deps || !branch || !loadIdentity) { + return + } + const seq = loadSeqRef.current + 1 + loadSeqRef.current = seq + // In-flight phase-2 work is NOT invalidated here: this load only takes phase-2 + // ownership when its own phase 2 actually starts (the bump below), so a load + // superseded at the phase-1 guard can never orphan a detailsSeq and silently + // discard the only details fetch. A stale ensure applying mid-phase-1 is safe — + // its identity/number/kind guards only let matching details through. + const previousIdentity = stateIdentityRef.current + stateIdentityRef.current = loadIdentity + // Soft refresh: same branch already showing ready/none stays visible while + // checks re-fetch (head advanced after commit). Hard loading only on first load + // or after a real identity wipe. + const keepVisible = + previousIdentity === loadIdentity && + (stateRef.current.kind === 'ready' || + stateRef.current.kind === 'none' || + stateRef.current.kind === 'loading') + if (!keepVisible) { + setState({ kind: 'loading' }) + } + // Phase 1: PR + checks (fast) — the worktree linkedPR read is parallelized with + // forBranch inside loadPrSidebarData so a closed/merged linked PR still resolves. + const next = await loadPrSidebarData(deps, { worktreeId, branch, headSha }) + if ( + !shouldApplyResult(seq, loadSeqRef.current) || + stateIdentityRef.current !== loadIdentity + ) { + return + } + stateIdentityRef.current = loadIdentity + + // Keep prior comments/body visible across phase 1 when the same PR is still open. + // loadPrSidebarData always returns details:null; without this, soft refresh and + // PR-tab refresh blank the comment tree until phase 2 finishes. + const priorDetails = + next.kind === 'ready' && + stateRef.current.kind === 'ready' && + stateRef.current.data.details != null && + stateRef.current.data.pr.number === next.data.pr.number + ? stateRef.current.data.details + : null + + if (next.kind === 'ready' && priorDetails != null) { + setState({ kind: 'ready', data: { ...next.data, details: priorDetails } }) + if (!includeDetails) { + return + } + } else { + setState(next) + if (next.kind !== 'ready' || !includeDetails) { + return + } + } + + // Phase 2: refresh (or first-load) the heavy comments/body payload. + const detailsSeq = detailsSeqRef.current + 1 + detailsSeqRef.current = detailsSeq + detailsInFlightRef.current = { seq: detailsSeq, prNumber: next.data.pr.number } + const fetchedDetails = await loadPrSidebarDetails(deps, worktreeId, next.data.pr.number) + // Release the claim unless a newer phase-2 superseded it (never clear theirs). + if (detailsInFlightRef.current?.seq === detailsSeq) { + detailsInFlightRef.current = null + } + // Phase-2 ownership is encoded by detailsSeq + identity + PR number alone — + // deliberately NOT by loadSeq: a chip-only soft refresh bumps loadSeq without + // bumping detailsSeq, and must not discard the in-flight details it preserved + // (ensure dedupes against this claim, so nothing would re-fetch them). + if ( + detailsSeq !== detailsSeqRef.current || + stateIdentityRef.current !== loadIdentity || + stateRef.current.kind !== 'ready' || + stateRef.current.data.pr.number !== next.data.pr.number + ) { + return + } + // Non-fatal null must not leave details===null (UI treats that as forever-loading). + const details = resolvePrSidebarDetailsAfterPhase2({ + fetched: fetchedDetails, + prior: stateRef.current.data.details, + pr: stateRef.current.data.pr + }) + setState({ kind: 'ready', data: { ...stateRef.current.data, details } }) + }, + [buildDeps, branch, headSha, identity, worktreeId] + ) + + // Phase-2 only — used when the hub opens the PR segment after a chip-only load. + // Uses detailsSeqRef (not loadSeqRef) so it cannot cancel a concurrent soft phase-1. + const ensurePrSidebarDetails = useCallback(async () => { + const current = stateRef.current + if (current.kind !== 'ready' || current.data.details != null) { + return + } const deps = buildDeps() const loadIdentity = identity - if (!deps || !branch || !loadIdentity) { + if (!deps || !loadIdentity || stateIdentityRef.current !== loadIdentity) { return } - const seq = loadSeqRef.current + 1 - loadSeqRef.current = seq - stateIdentityRef.current = loadIdentity - setState({ kind: 'loading' }) - // Phase 1: PR + checks (fast) — the worktree linkedPR read is parallelized with - // forBranch inside loadPrSidebarData so a closed/merged linked PR still resolves. - const next = await loadPrSidebarData(deps, { worktreeId, branch, headSha }) - if (!shouldApplyResult(seq, loadSeqRef.current) || stateIdentityRef.current !== loadIdentity) { + const prNumber = current.data.pr.number + // A live phase-2 fetch for this PR is already in flight (its claim still owns + // the latest details seq) — do not start a duplicate. + const inFlight = detailsInFlightRef.current + if (inFlight && inFlight.prNumber === prNumber && inFlight.seq === detailsSeqRef.current) { return } - stateIdentityRef.current = loadIdentity - setState(next) - if (next.kind !== 'ready') { + const detailsSeq = detailsSeqRef.current + 1 + detailsSeqRef.current = detailsSeq + detailsInFlightRef.current = { seq: detailsSeq, prNumber } + const fetchedDetails = await loadPrSidebarDetails(deps, worktreeId, prNumber) + if (detailsInFlightRef.current?.seq === detailsSeq) { + detailsInFlightRef.current = null + } + if ( + detailsSeq !== detailsSeqRef.current || + stateIdentityRef.current !== loadIdentity || + stateRef.current.kind !== 'ready' || + stateRef.current.data.pr.number !== prNumber + ) { return } - // Phase 2: lazy-load the heavy comments/body payload and merge it in, so it never - // blocks the actionable PR UI. Re-check the seq so a newer load isn't clobbered. - const details = await loadPrSidebarDetails(deps, worktreeId, next.data.pr.number) - if (shouldApplyResult(seq, loadSeqRef.current) && stateIdentityRef.current === loadIdentity) { - setState({ kind: 'ready', data: { ...next.data, details } }) + const details = resolvePrSidebarDetailsAfterPhase2({ + fetched: fetchedDetails, + prior: stateRef.current.data.details, + pr: stateRef.current.data.pr + }) + setState({ + kind: 'ready', + data: { ...stateRef.current.data, details } + }) + }, [buildDeps, identity, worktreeId]) + + // Soft-refresh checks when HEAD advances on the same branch (post-commit). + // Also restarts an in-flight phase-1 load so a mid-flight head advance is not + // applied with a stale SHA (headShaRef would otherwise advance with no reload). + useEffect(() => { + if (headShaRef.current === headSha) { + return } - }, [buildDeps, branch, headSha, identity, worktreeId]) + headShaRef.current = headSha + // Identity was just wiped (branch/worktree switch): stateRef still holds the + // pre-wipe state in this effect flush, which would start a load flavored for + // the OLD surface (e.g. heavy details on the Changes tab). Let the owning + // surface's hidden-state effects drive the first load for the new identity. + if (stateIdentityRef.current === null) { + return + } + const current = stateRef.current + if (!shouldSoftRefreshPrSidebarOnHeadChange(current.kind)) { + return + } + void load({ + includeDetails: current.kind === 'ready' && current.data.details != null + }) + }, [headSha, load]) const openPRSidebar = useCallback(() => { setShowPRSidebar(true) @@ -131,18 +298,26 @@ export function useMobilePrSidebarController(input: PrSidebarControllerInput) { stateIdentityRef.current !== identity || (state.kind !== 'ready' && state.kind !== 'loading') ) { - void load() + void load({ includeDetails: true }) + } else if (state.kind === 'ready' && state.data.details == null) { + void ensurePrSidebarDetails() } - }, [identity, state.kind, load]) + }, [identity, state, load, ensurePrSidebarDetails]) + + const retry = useCallback(() => { + void load({ includeDetails: true }) + }, [load]) return { prSidebarState: state, prSidebarIsGithubRepo: isGithubRepo, + prSidebarRepoProbeLoaded: repoProbeLoaded, showPRSidebar, setShowPRSidebar, openPRSidebar, - retryPRSidebar: load, - refetchPRSidebar: load + retryPRSidebar: retry, + refetchPRSidebar: load, + ensurePrSidebarDetails } } diff --git a/mobile/src/source-control/MobileGitHistoryList.tsx b/mobile/src/source-control/MobileGitHistoryList.tsx index e4856be152f..db12d3117f1 100644 --- a/mobile/src/source-control/MobileGitHistoryList.tsx +++ b/mobile/src/source-control/MobileGitHistoryList.tsx @@ -1,4 +1,4 @@ -import { useCallback, useEffect, useState } from 'react' +import { memo, useCallback, useEffect, useState } from 'react' import { ActivityIndicator, FlatList, Pressable, StyleSheet, Text, View } from 'react-native' import { ChevronDown, ChevronRight } from 'lucide-react-native' import { colors, radii, spacing, typography } from '../theme/mobile-theme' @@ -26,7 +26,9 @@ type Props = { // Headerless commit-history list. Extracted from the /history route so the hub's // History segment and the standalone route render the same body over one code path. -export function MobileGitHistoryList({ +// Memoized: it stays mounted (hidden) while the Changes segment is active, and must +// not re-reconcile its FlatList on every commit-message keystroke re-render. +export const MobileGitHistoryList = memo(function MobileGitHistoryList({ client, connState, worktreeId, @@ -217,7 +219,7 @@ export function MobileGitHistoryList({ contentContainerStyle={{ paddingBottom: spacing.lg + bottomInset }} /> ) -} +}) const styles = StyleSheet.create({ state: { flex: 1, alignItems: 'center', justifyContent: 'center', padding: spacing.lg }, diff --git a/mobile/src/source-control/MobileSourceControlBranchCard.tsx b/mobile/src/source-control/MobileSourceControlBranchCard.tsx index f7135347fd0..04aae01b0bc 100644 --- a/mobile/src/source-control/MobileSourceControlBranchCard.tsx +++ b/mobile/src/source-control/MobileSourceControlBranchCard.tsx @@ -4,6 +4,7 @@ import { colors } from '../theme/mobile-theme' import { styles } from './mobile-source-control-styles' import { MobileSourceControlPrChip } from './MobileSourceControlPrChip' import type { MobilePrChipSummary } from './mobile-pr-chip-summary' +import { mobileConflictAbortLabel } from './mobile-source-control-conflict-abort' type Props = { branchLabel: string @@ -12,7 +13,10 @@ type Props = { stagedCount: number branchCount: number conflictOperation: string | null + // True while any serial git IO is in flight — disables Abort so ops don't race. conflictBusy: boolean + // True only while abort-merge / abort-rebase itself is running (label accuracy). + conflictAborting: boolean onAbortConflict: (operation: string) => void // The PR chip is shown only on repos with a hosted-review remote; null hides it. prChip: MobilePrChipSummary | null @@ -30,6 +34,7 @@ export function MobileSourceControlBranchCard({ branchCount, conflictOperation, conflictBusy, + conflictAborting, onAbortConflict, prChip, onOpenPr @@ -60,7 +65,7 @@ export function MobileSourceControlBranchCard({ onPress={() => onAbortConflict(conflictOperation)} > - {conflictBusy ? 'Aborting…' : `Abort ${conflictOperation}`} + {mobileConflictAbortLabel(conflictOperation, conflictAborting)} ) : null} diff --git a/mobile/src/source-control/MobileSourceControlHeader.tsx b/mobile/src/source-control/MobileSourceControlHeader.tsx index 5d6de2d93be..4485f2ac803 100644 --- a/mobile/src/source-control/MobileSourceControlHeader.tsx +++ b/mobile/src/source-control/MobileSourceControlHeader.tsx @@ -1,5 +1,5 @@ import { Pressable, Text, View } from 'react-native' -import { ChevronLeft, RefreshCw, X } from 'lucide-react-native' +import { ChevronLeft, ExternalLink, RefreshCw, X } from 'lucide-react-native' import { colors } from '../theme/mobile-theme' import { styles } from './mobile-source-control-styles' @@ -9,6 +9,10 @@ type Props = { ioBusy: boolean onBack: () => void onRefresh: () => void + // When set (PR segment ready with a host URL), show open-on-web flush-right of + // the title so the control stays visible while the PR body scrolls. + onOpenPrWeb?: () => void + prNumber?: number | null } export function MobileSourceControlHeader({ @@ -16,7 +20,9 @@ export function MobileSourceControlHeader({ worktreeLabel, ioBusy, onBack, - onRefresh + onRefresh, + onOpenPrWeb, + prNumber = null }: Props) { return ( @@ -40,6 +46,21 @@ export function MobileSourceControlHeader({ {worktreeLabel} + {onOpenPrWeb ? ( + [styles.refreshButton, pressed && styles.refreshButtonPressed]} + onPress={onOpenPrWeb} + hitSlop={8} + accessibilityRole="link" + accessibilityLabel={ + prNumber != null + ? `Open pull request #${prNumber} on the web` + : 'Open pull request on the web' + } + > + + + ) : null} [ styles.refreshButton, diff --git a/mobile/src/source-control/MobileSourceControlPanel.tsx b/mobile/src/source-control/MobileSourceControlPanel.tsx index 3d0973b7f12..e16c2d05388 100644 --- a/mobile/src/source-control/MobileSourceControlPanel.tsx +++ b/mobile/src/source-control/MobileSourceControlPanel.tsx @@ -1,4 +1,4 @@ -import { useCallback, useEffect, useMemo, useState } from 'react' +import { useCallback, useEffect, useMemo, useRef, useState } from 'react' import { ActivityIndicator, Pressable, Text, View } from 'react-native' import { SafeAreaView } from 'react-native-safe-area-context' import { colors } from '../theme/mobile-theme' @@ -14,8 +14,10 @@ import { styles } from './mobile-source-control-styles' import { hubStyles } from './mobile-source-control-hub-styles' import type { SourceControlHubTab } from './mobile-source-control-hub-tab' import { buildMobilePrChipSummary, countUnresolvedReviewThreads } from './mobile-pr-chip-summary' +import { isMobileConflictAborting } from './mobile-source-control-conflict-abort' import { useMobilePrSidebarController } from '../session/use-mobile-pr-sidebar-controller' import { MobilePrViewPanelBody } from '../components/pr-sidebar/MobilePrViewPanel' +import { openMobilePrUrl } from '../components/MobilePrComposeSheet' export type MobileSourceControlPanelProps = { hostId: string @@ -43,8 +45,9 @@ export function MobileSourceControlPanel({ onOpenedFileDiff }: MobileSourceControlPanelProps) { const [activeTab, setActiveTab] = useState(initialTab) - // Track first visit so PR/History keep their fetch + scroll state across segment - // switches without paying the mount cost until the user actually opens them. + // Track first visit so Changes/History keep scroll state across segment switches + // without paying the mount cost until the user actually opens them. PR unmounts + // when inactive (WebViews / comment tree) but its controller state stays for the chip. const [visitedTabs, setVisitedTabs] = useState>( () => new Set([initialTab]) ) @@ -104,6 +107,7 @@ export function MobileSourceControlPanel({ openingBranchPath, loadStatus, status, + branchCompareResult, branchLabel, syncLabel, unstagedCount, @@ -118,8 +122,25 @@ export function MobileSourceControlPanel({ // segment, so the chip's rollup can never disagree with the checks list it // links to. Branch + head come from the already-loaded git.status — no second // status read. The chip loads independently, so it never blocks the file list. - const prBranch = status?.branch ?? null - const prHeadSha = status?.head ?? null + // Keep last-known identity across a transient status unload (disconnect / + // failed refresh) so the controller does not wipe ready → hidden → cold start. + // Head SHA matches the review path: status.head ?? branchCompare headOid. + const lastPrBranchRef = useRef(null) + const lastPrHeadRef = useRef(null) + useEffect(() => { + lastPrBranchRef.current = null + lastPrHeadRef.current = null + }, [worktreeId]) + const statusBranch = status?.branch ?? null + const statusHead = status?.head ?? branchCompareResult?.summary.headOid ?? null + if (statusBranch) { + lastPrBranchRef.current = statusBranch + } + if (statusHead) { + lastPrHeadRef.current = statusHead + } + const prBranch = statusBranch ?? lastPrBranchRef.current + const prHeadSha = statusHead ?? lastPrHeadRef.current const prController = useMobilePrSidebarController({ client, connState, @@ -130,11 +151,49 @@ export function MobileSourceControlPanel({ const isHostedRepo = prController.prSidebarIsGithubRepo const prSidebarKind = prController.prSidebarState.kind const refetchPr = prController.refetchPRSidebar + const ensurePrDetails = prController.ensurePrSidebarDetails + // Refs so tab effects do not re-fire when headSha recreates load() (soft refresh). + const refetchPrRef = useRef(refetchPr) + refetchPrRef.current = refetchPr + const ensurePrDetailsRef = useRef(ensurePrDetails) + ensurePrDetailsRef.current = ensurePrDetails + + // Chip bootstrap: phase-1 only (PR + checks). Full comment payload waits until + // the Pull Request segment is open — opening SC to stage must not pull details. useEffect(() => { - if (prBranch && isHostedRepo && prSidebarKind === 'hidden') { - refetchPr() + if (activeTab === 'pr') { + return } - }, [prBranch, isHostedRepo, prSidebarKind, refetchPr]) + if (prBranch && isHostedRepo && prSidebarKind === 'hidden') { + void refetchPrRef.current({ includeDetails: false }) + } + }, [activeTab, prBranch, isHostedRepo, prSidebarKind]) + + // Number of the ready PR whose phase-2 details are still missing (null when + // none). Keyed by PR number — not a boolean — so a same-branch PR swap during a + // chip-only soft refresh re-arms phase 2 for the new PR instead of leaving its + // comments on a forever-spinner (a stale ensure bails on the number mismatch). + const prDetailsMissingFor = + prController.prSidebarState.kind === 'ready' && prController.prSidebarState.data.details == null + ? prController.prSidebarState.data.pr.number + : null + + // PR segment: full load or phase-2 fill-in. Body only mounts while active. + // Guarded (and keyed) on prBranch so a branch that arrives while the segment is + // already open — e.g. mounted on a detached HEAD, then checkout — still loads; + // kind stays 'hidden' through that transition, so kind alone can't re-fire this. + useEffect(() => { + if (activeTab !== 'pr' || !isHostedRepo || !prBranch) { + return + } + if (prSidebarKind === 'hidden') { + void refetchPrRef.current({ includeDetails: true }) + return + } + if (prDetailsMissingFor != null) { + void ensurePrDetailsRef.current() + } + }, [activeTab, isHostedRepo, prBranch, prSidebarKind, prDetailsMissingFor]) const prChip = useMemo(() => { if (!isHostedRepo) { @@ -147,21 +206,39 @@ export function MobileSourceControlPanel({ return buildMobilePrChipSummary(prController.prSidebarState, commentCount) }, [isHostedRepo, prController.prSidebarState]) + // Design: refresh the active segment's body work, plus git.status for the shared + // branch card (counts/sync stay honest even while on History). Preserve ready + // status on a failed refresh so PR chip identity is not wiped to hidden. const onRefresh = useCallback(() => { - void loadStatus() - // Chip + PR segment share the controller; always refresh when hosted so the - // branch card stays in sync even while the Changes segment is active. - if (isHostedRepo) { - void refetchPr() - } - if (visitedTabs.has('history')) { + void loadStatus({ preserveReadyOnFailure: true }) + if (activeTab === 'history') { setHistoryRefreshNonce((n) => n + 1) + return } - }, [isHostedRepo, loadStatus, refetchPr, visitedTabs]) + if (!isHostedRepo) { + return + } + if (activeTab === 'pr') { + void refetchPr({ includeDetails: true }) + return + } + // Changes: light chip refresh so the branch card stays current without comments. + void refetchPr({ includeDetails: false }) + }, [activeTab, isHostedRepo, loadStatus, refetchPr]) // Embedded mode docks beside the terminal: close the dock instead of popping // a route, and skip the full-screen safe-area chrome (the dock column owns it). const onBack = embedded ? (onRequestClose ?? (() => router.back())) : () => router.back() + // Chromeless PR body has no panel header — surface open-on-web on the hub chrome + // while the Pull Request segment is active (same affordance as the old /pr route). + const prWebUrl = + activeTab === 'pr' && + prController.prSidebarState.kind === 'ready' && + prController.prSidebarState.data.pr.url + ? prController.prSidebarState.data.pr.url + : null + const prWebNumber = + prController.prSidebarState.kind === 'ready' ? prController.prSidebarState.data.pr.number : null const header = ( openMobilePrUrl(prWebUrl) : undefined} + prNumber={prWebNumber} /> ) @@ -206,8 +285,14 @@ export function MobileSourceControlPanel({ // History only needs the RPC client — do not block it behind git.status. // Changes/PR need status (branch, file list, head SHA), so they stay gated. + const showChanges = ready && (activeTab === 'changes' || visitedTabs.has('changes')) const showHistory = activeTab === 'history' || visitedTabs.has('history') - const showPr = ready && (activeTab === 'pr' || visitedTabs.has('pr')) + // Why: only mount the PR body while its segment is active. Keep-mounting retained + // Mermaid WebViews and re-rendered the comment tree on every commit keystroke. + // Controller + chip state still live for instant re-open without a full cold start. + const showPrBody = ready && activeTab === 'pr' + const conflictOperation = status?.conflictOperation ?? null + const conflictAborting = isMobileConflictAborting(busyAction, conflictOperation) return ( @@ -228,24 +313,25 @@ export function MobileSourceControlPanel({ unstagedCount={unstagedCount} stagedCount={stagedCount} branchCount={branchEntries.length} - conflictOperation={status?.conflictOperation ?? null} + conflictOperation={conflictOperation} conflictBusy={busyAction !== null} + conflictAborting={conflictAborting} onAbortConflict={(operation) => void abortConflictOperation(operation)} prChip={prChip} onOpenPr={openPrTab} /> ) : null} - {activeTab === 'changes' ? ( - ready ? ( + {showChanges ? ( + - ) : ( - statusGate - ) + + ) : activeTab === 'changes' ? ( + statusGate ) : null} - {showPr ? ( - + {showPrBody ? ( + ) : activeTab === 'pr' ? ( diff --git a/mobile/src/source-control/mobile-source-control-conflict-abort.test.ts b/mobile/src/source-control/mobile-source-control-conflict-abort.test.ts new file mode 100644 index 00000000000..fa9e40a4746 --- /dev/null +++ b/mobile/src/source-control/mobile-source-control-conflict-abort.test.ts @@ -0,0 +1,28 @@ +import { describe, expect, it } from 'vitest' +import { + isMobileConflictAborting, + mobileConflictAbortLabel +} from './mobile-source-control-conflict-abort' + +describe('isMobileConflictAborting', () => { + it('is true only for the matching abort action', () => { + expect(isMobileConflictAborting('abort-merge', 'merge')).toBe(true) + expect(isMobileConflictAborting('abort-rebase', 'rebase')).toBe(true) + }) + + it('is false for other busy actions (stage/commit must not look like abort)', () => { + expect(isMobileConflictAborting('stage-all', 'merge')).toBe(false) + expect(isMobileConflictAborting('commit', 'rebase')).toBe(false) + expect(isMobileConflictAborting(null, 'merge')).toBe(false) + expect(isMobileConflictAborting('abort-merge', 'rebase')).toBe(false) + expect(isMobileConflictAborting('abort-merge', 'unknown')).toBe(false) + }) +}) + +describe('mobileConflictAbortLabel', () => { + it('shows Aborting only while abort is in flight', () => { + expect(mobileConflictAbortLabel('merge', true)).toBe('Aborting…') + expect(mobileConflictAbortLabel('merge', false)).toBe('Abort merge') + expect(mobileConflictAbortLabel('rebase', false)).toBe('Abort rebase') + }) +}) diff --git a/mobile/src/source-control/mobile-source-control-conflict-abort.ts b/mobile/src/source-control/mobile-source-control-conflict-abort.ts new file mode 100644 index 00000000000..ac71bcbf9ed --- /dev/null +++ b/mobile/src/source-control/mobile-source-control-conflict-abort.ts @@ -0,0 +1,18 @@ +// Pure helpers for the branch-card conflict Abort control. Kept free of React so +// the busy-label rule (abort-in-flight only) is unit-testable. + +/** True while git.abortMerge / git.abortRebase is the active serial action. */ +export function isMobileConflictAborting( + busyAction: string | null, + conflictOperation: string | null +): boolean { + if (conflictOperation !== 'merge' && conflictOperation !== 'rebase') { + return false + } + return busyAction === `abort-${conflictOperation}` +} + +/** Label for the Abort control — never says "Aborting…" for unrelated busy work. */ +export function mobileConflictAbortLabel(conflictOperation: string, aborting: boolean): string { + return aborting ? 'Aborting…' : `Abort ${conflictOperation}` +}