refactor(app): simplify Changes controls

This commit is contained in:
Mohamed Boudra
2026-08-17 21:15:16 +02:00
parent 5110e6e92a
commit 8655722296
2 changed files with 208 additions and 269 deletions
+30 -16
View File
@@ -596,32 +596,41 @@ test("Changes keeps review navigation and controls inside its workspace tab", as
await expect(visiblePanel).toBeVisible();
await expect(visiblePanel.getByText("use-mounted-tab-set.ts", { exact: true })).toBeVisible();
await expect(visiblePanel).toContainText("zz-deleted.ts");
await expect(visiblePanel.getByTestId("changes-open-tab")).toHaveCount(0);
await expect(visiblePanel.getByTestId("changes-primary-cta")).toHaveCount(0);
await expect(page.getByTestId("changes-primary-cta")).toHaveCount(1);
await expect(page.getByTestId("changes-primary-cta")).toContainText("Commit");
await expect(visiblePanel.getByTestId("diff-file-0-body")).toBeVisible();
await visiblePanel.getByTestId("diff-file-0-toggle").click();
await expect(visiblePanel.getByTestId("diff-file-0-body")).not.toBeVisible();
await visiblePanel.getByRole("button", { name: "Collapse all files" }).click();
await expect(visiblePanel.getByRole("button", { name: "Expand all files" })).toBeVisible();
await visiblePanel.getByRole("button", { name: "Expand all files" }).click();
// Collapse-all and the layout switch are overflow-menu items, not toolbar icons.
const diffOptions = visiblePanel.getByRole("button", { name: "Diff options" });
await expect(page.getByTestId("changes-toggle-collapse-all")).toHaveCount(0);
await diffOptions.click();
// A workspace tab has nowhere to open itself, so the menu withholds that item.
await expect(page.getByTestId("changes-open-tab")).toHaveCount(0);
await page.getByTestId("changes-toggle-collapse-all").click();
await diffOptions.click();
await expect(page.getByTestId("changes-toggle-collapse-all")).toContainText("Expand all files");
await page.getByTestId("changes-toggle-collapse-all").click();
await expect(visiblePanel.getByTestId("diff-file-0-body")).toBeVisible();
const workingDiffLayoutToggle = visiblePanel.getByRole("button", {
name: "Switch to side-by-side diff",
});
await expect(workingDiffLayoutToggle).toHaveAccessibleName("Switch to side-by-side diff");
await workingDiffLayoutToggle.click();
await expect(visiblePanel.getByRole("button", { name: "Switch to unified diff" })).toBeVisible();
await visiblePanel.getByRole("button", { name: "Diff options" }).click();
const layoutItem = page.getByTestId("changes-toggle-layout");
await diffOptions.click();
await expect(layoutItem).toContainText("Side-by-side diff");
// Leading icon only: the layout is still unified, so no trailing check.
await expect(layoutItem.locator("svg")).toHaveCount(1);
await layoutItem.click();
await diffOptions.click();
await expect(layoutItem.locator("svg")).toHaveCount(2);
await expect(page.getByText("Hide whitespace", { exact: true })).toBeVisible();
await expect(page.getByText("Wrap long lines", { exact: true })).toBeVisible();
await expect(page.getByText(/^Refresh/)).toBeVisible();
await expect(page.getByTestId("changes-refresh")).toHaveText("Refresh");
await page.getByText("Wrap long lines", { exact: true }).click();
await visiblePanel.getByRole("button", { name: "Diff options" }).click();
await diffOptions.click();
await expect(page.getByText("Scroll long lines", { exact: true })).toBeVisible();
await page.keyboard.press("Escape");
await expect(visiblePanel.getByRole("button", { name: "Collapse all files" })).toBeVisible();
await expect(page.getByTestId(/^workspace-working-diff-close-/)).toHaveCount(1);
await writeFile(path.join(workspace.repoPath, "src/use-mounted-tab-set.ts"), BEFORE);
@@ -645,15 +654,20 @@ test("desktop Changes toggles a navigation tree beside the expanded diff documen
await openWorkspaceChanges(page, workspace);
await expectFlatFileList(page);
await expect(page.getByTestId("changes-toggle-layout")).toBeVisible();
await expect(page.getByTestId("changes-layout-unified")).toHaveCount(0);
await expect(page.getByTestId("changes-layout-split")).toHaveCount(0);
// The tree toggle is the one icon action that stays outside the overflow menu.
await expect(page.getByTestId("changes-toggle-tree")).toBeVisible();
await expect(page.getByTestId("changes-toggle-collapse-all")).toHaveCount(0);
await expect(page.getByTestId("changes-toggle-layout")).toHaveCount(0);
await page.getByTestId("changes-options-menu").click();
await expect(page.getByTestId("changes-options-menu-content")).toBeVisible();
await expect(page.getByTestId("changes-toggle-collapse-all")).toContainText("Collapse all files");
await expect(page.getByTestId("changes-toggle-layout")).toContainText("Side-by-side diff");
await expect(page.getByTestId("changes-toggle-whitespace")).toContainText("Hide whitespace");
await expect(page.getByTestId("changes-toggle-wrap-lines")).toContainText("Wrap long lines");
await expect(page.getByTestId("changes-refresh")).toContainText("Refresh");
await expect(page.getByTestId("changes-refresh")).toHaveText("Refresh");
await page.getByTestId("changes-toggle-whitespace").click();
await page.getByTestId("changes-options-menu").click();
await expect(page.getByTestId("changes-toggle-whitespace")).toContainText("Show whitespace");
+143 -218
View File
@@ -2,6 +2,7 @@ import { useState, useCallback, useMemo, type ReactElement } from "react";
import { useTranslation } from "react-i18next";
import type { TFunction } from "i18next";
import { TreeRail } from "@/components/tree-rail";
import { DiffStat } from "@/components/diff-stat";
import {
View,
Text,
@@ -22,6 +23,7 @@ import {
ListChevronsDownUp,
ListChevronsUpDown,
Maximize2,
MoreHorizontal,
Pilcrow,
RotateCw,
WrapText,
@@ -84,6 +86,22 @@ export function resolveDiffLayout(
return canUseSplitLayout ? layout : "unified";
}
function computeSelectedDiffStat(
files: ParsedDiffFile[],
isLoading: boolean,
): { additions: number; deletions: number } | null {
if (isLoading) {
return null;
}
return files.reduce(
(total, file) => ({
additions: total.additions + file.additions,
deletions: total.deletions + file.deletions,
}),
{ additions: 0, deletions: 0 },
);
}
function useDiscardChangesAction({
serverId,
cwd,
@@ -163,12 +181,25 @@ const ThemedListChevronsDownUp = withUnistyles(ListChevronsDownUp);
const ThemedListChevronsUpDown = withUnistyles(ListChevronsUpDown);
const ThemedMaximize2 = withUnistyles(Maximize2);
const ThemedChevronDown = withUnistyles(ChevronDown);
const ThemedMoreHorizontal = withUnistyles(MoreHorizontal);
const DIFF_OPTIONS_WHITESPACE_ICON = (
<ThemedPilcrow size={14} uniProps={foregroundMutedIconColorMapping} />
);
const DIFF_OPTIONS_WRAP_ICON = (
<ThemedWrapText size={14} uniProps={foregroundMutedIconColorMapping} />
);
const DIFF_OPTIONS_SPLIT_ICON = (
<ThemedColumns2 size={14} uniProps={foregroundMutedIconColorMapping} />
);
const DIFF_OPTIONS_COLLAPSE_ICON = (
<ThemedListChevronsDownUp size={14} uniProps={foregroundMutedIconColorMapping} />
);
const DIFF_OPTIONS_EXPAND_ICON = (
<ThemedListChevronsUpDown size={14} uniProps={foregroundMutedIconColorMapping} />
);
const DIFF_OPTIONS_CHANGES_TAB_ICON = (
<ThemedMaximize2 size={14} uniProps={foregroundMutedIconColorMapping} />
);
interface DiffLayoutToggleProps {
layout: "unified" | "split";
@@ -220,12 +251,6 @@ export function DiffLayoutToggle({
);
}
interface ChangesTabToggleProps {
isMobile: boolean;
selected: boolean;
onPress: () => void;
}
function resolveChangesTabOpen(host: "explorer" | "panel", changesTabOpen: boolean): boolean {
return host === "explorer" ? changesTabOpen : false;
}
@@ -237,18 +262,6 @@ function resolveChangesFilePress(
return host === "explorer" ? onChangesFilePress : undefined;
}
function ChangesTabToggleForHost({
host,
isMobile,
selected,
onPress,
}: ChangesTabToggleProps & { host: "explorer" | "panel" }) {
if (host === "panel") {
return null;
}
return <ChangesTabToggle isMobile={isMobile} selected={selected} onPress={onPress} />;
}
interface DiffModeMenuProps {
diffMode: "uncommitted" | "base";
committedDescription?: string;
@@ -303,38 +316,6 @@ export function DiffModeMenu({
);
}
function ChangesTabToggle({ isMobile, selected, onPress }: ChangesTabToggleProps) {
const { t } = useTranslation();
const buttonStyle = useMemo(
() => buildToggleButtonStyle(selected, styles.toolbarIconButton),
[selected],
);
const label = t(
selected ? "workspace.git.diff.closeChangesTab" : "workspace.git.diff.openChangesTab",
);
if (isMobile) {
return null;
}
return (
<Tooltip delayDuration={300}>
<TooltipTrigger asChild>
<Pressable
accessibilityRole="button"
accessibilityLabel={label}
testID="changes-open-tab"
onPress={onPress}
style={buttonStyle}
>
<ThemedMaximize2 size={14} uniProps={foregroundMutedIconColorMapping} />
</Pressable>
</TooltipTrigger>
<TooltipContent side="bottom">
<Text style={styles.tooltipText}>{label}</Text>
</TooltipContent>
</Tooltip>
);
}
interface DesktopTreeToggleProps {
visible: boolean;
toggleStyle?: PressableStyleFn;
@@ -368,54 +349,6 @@ function DesktopTreeToggle({ visible, toggleStyle, onToggle }: DesktopTreeToggle
);
}
interface CollapseAllToggleProps {
allCollapsed: boolean;
isMobile: boolean;
onCollapseAll: () => void;
onExpandAll: () => void;
}
function CollapseAllToggle({
allCollapsed,
isMobile,
onCollapseAll,
onExpandAll,
}: CollapseAllToggleProps) {
const { t } = useTranslation();
const buttonStyle = useMemo(() => buildToggleButtonStyle(false, styles.toolbarIconButton), []);
const label = t(
allCollapsed ? "workspace.git.diff.expandAllFiles" : "workspace.git.diff.collapseAllFiles",
);
return (
<Tooltip delayDuration={300}>
<TooltipTrigger asChild>
<Pressable
accessibilityRole="button"
accessibilityLabel={label}
testID="changes-toggle-collapse-all"
style={buttonStyle}
onPress={allCollapsed ? onExpandAll : onCollapseAll}
>
{allCollapsed ? (
<ThemedListChevronsUpDown
size={isMobile ? 18 : 14}
uniProps={foregroundMutedIconColorMapping}
/>
) : (
<ThemedListChevronsDownUp
size={isMobile ? 18 : 14}
uniProps={foregroundMutedIconColorMapping}
/>
)}
</Pressable>
</TooltipTrigger>
<TooltipContent side="bottom">
<Text style={styles.tooltipText}>{label}</Text>
</TooltipContent>
</Tooltip>
);
}
interface ChangesToolbarProps {
branchName: string | null;
allFilesCollapsed: boolean;
@@ -425,7 +358,6 @@ interface ChangesToolbarProps {
cwd: string;
desktopTreeVisible: boolean;
diffMode: "uncommitted" | "base";
forge: Forge;
gitActions: GitActions;
hasFiles: boolean;
hideWhitespace: boolean;
@@ -433,9 +365,9 @@ interface ChangesToolbarProps {
isMobile: boolean;
isRefreshing: boolean;
layout: "unified" | "split";
layoutToggleStyle: PressableStyleFn;
overflowToggleStyle: PressableStyleFn;
refreshSupported: boolean;
selectedDiffStat: { additions: number; deletions: number } | null;
serverId: string;
treeToggleStyle: PressableStyleFn;
workspaceId?: string | null;
@@ -452,44 +384,30 @@ interface ChangesToolbarProps {
onToggleWrapLines: () => void;
}
function ChangesToolbar({
// One row: the diff-mode and branch pickers lead, the git actions and the
// overflow menu trail. The tree toggle is the only icon action that stays out
// of the menu, and it only exists on desktop, so a phone-width row holds two
// pickers, the split button, and the trigger without wrapping.
function ChangesToolbar(props: ChangesToolbarProps) {
const {
branchName,
allFilesCollapsed,
canUseSplitLayout,
changesTabOpen,
committedDescription,
cwd,
desktopTreeVisible,
diffMode,
forge,
gitActions,
hasFiles,
hideWhitespace,
host,
isMobile,
isRefreshing,
layout,
layoutToggleStyle,
overflowToggleStyle,
refreshSupported,
selectedDiffStat,
serverId,
treeToggleStyle,
workspaceId,
wrapLines,
onRefresh,
onCollapseAll,
onExpandAll,
onSelectBase,
onSelectUncommitted,
onToggleChangesTab,
onToggleDesktopTree,
onToggleHideWhitespace,
onToggleLayout,
onToggleWrapLines,
}: ChangesToolbarProps) {
} = props;
return (
<View style={styles.changesToolbar} testID="changes-header">
<View style={styles.changesToolbarMainRow}>
<View style={styles.changesToolbarIdentity}>
<DiffModeMenu
diffMode={diffMode}
@@ -505,30 +423,15 @@ function ChangesToolbar({
isGitCheckout
testID="changes-branch-switcher"
/>
{!isMobile && selectedDiffStat ? (
<DiffStat
additions={selectedDiffStat.additions}
deletions={selectedDiffStat.deletions}
testID="changes-selected-diff-stat"
/>
) : null}
</View>
<View style={styles.changesToolbarControls}>
<ChangesTabToggleForHost
host={host}
isMobile={isMobile}
selected={changesTabOpen}
onPress={onToggleChangesTab}
/>
{hasFiles ? (
<CollapseAllToggle
allCollapsed={allFilesCollapsed}
isMobile={isMobile}
onCollapseAll={onCollapseAll}
onExpandAll={onExpandAll}
/>
) : null}
{canUseSplitLayout && !changesTabOpen ? (
<DiffLayoutToggle
layout={layout}
isMobile={isMobile}
toggleStyle={layoutToggleStyle}
onToggle={onToggleLayout}
/>
) : null}
{!isMobile && hasFiles ? (
<DesktopTreeToggle
visible={desktopTreeVisible}
@@ -536,71 +439,74 @@ function ChangesToolbar({
onToggle={onToggleDesktopTree}
/>
) : null}
<DiffOptionsMenu
brand={getForgePresentation(forge).brandLabel}
hideWhitespace={hideWhitespace}
isMobile={isMobile}
isRefreshing={isRefreshing}
overflowToggleStyle={overflowToggleStyle}
refreshSupported={refreshSupported}
wrapLines={wrapLines}
onRefresh={onRefresh}
onToggleHideWhitespace={onToggleHideWhitespace}
onToggleWrapLines={onToggleWrapLines}
/>
{isMobile ? <GitActionsSplitButton gitActions={gitActions} /> : null}
<ChangesOptionsMenu {...props} />
</View>
</View>
{isMobile ? (
<View style={styles.mobileGitActionsRow}>
<GitActionsSplitButton gitActions={gitActions} />
</View>
) : null}
</View>
);
}
interface DiffOptionsMenuProps {
brand?: string;
hideWhitespace: boolean;
isMobile: boolean;
isRefreshing?: boolean;
overflowToggleStyle?: PressableStyleFn;
refreshSupported?: boolean;
testIDPrefix?: string;
wrapLines: boolean;
onRefresh?: () => void;
onToggleHideWhitespace: () => void;
onToggleWrapLines: () => void;
}
type ChangesOptionsMenuProps = Pick<
ChangesToolbarProps,
| "allFilesCollapsed"
| "canUseSplitLayout"
| "changesTabOpen"
| "hasFiles"
| "hideWhitespace"
| "host"
| "isMobile"
| "isRefreshing"
| "layout"
| "overflowToggleStyle"
| "refreshSupported"
| "wrapLines"
| "onCollapseAll"
| "onExpandAll"
| "onRefresh"
| "onToggleChangesTab"
| "onToggleHideWhitespace"
| "onToggleLayout"
| "onToggleWrapLines"
>;
export function DiffOptionsMenu({
brand,
function ChangesOptionsMenu({
allFilesCollapsed,
canUseSplitLayout,
changesTabOpen,
hasFiles,
hideWhitespace,
host,
isMobile,
isRefreshing = false,
isRefreshing,
layout,
overflowToggleStyle,
refreshSupported = false,
testIDPrefix = "changes",
refreshSupported,
wrapLines,
onCollapseAll,
onExpandAll,
onRefresh,
onToggleChangesTab,
onToggleHideWhitespace,
onToggleLayout,
onToggleWrapLines,
}: DiffOptionsMenuProps) {
}: ChangesOptionsMenuProps) {
const { t } = useTranslation();
const defaultToggleStyle = useMemo(() => buildOverflowButtonStyle(), []);
const optionsLabel = t("workspace.git.diff.options");
const collapseLabel = t(
allFilesCollapsed ? "workspace.git.diff.expandAllFiles" : "workspace.git.diff.collapseAllFiles",
);
const changesTabLabel = t(
changesTabOpen ? "workspace.git.diff.closeChangesTab" : "workspace.git.diff.openChangesTab",
);
const whitespaceLabel = hideWhitespace
? t("workspace.git.diff.showWhitespace")
: t("workspace.git.diff.hideWhitespace");
const wrapLinesLabel = wrapLines
? t("workspace.git.diff.scrollLongLines")
: t("workspace.git.diff.wrapLongLines");
const optionsLabel = t("workspace.git.diff.options");
let refreshLabel = t("workspace.git.diff.refresh");
if (isRefreshing) {
refreshLabel = t("workspace.git.diff.refreshing");
} else if (brand) {
refreshLabel = t("workspace.git.diff.refreshState", { brand });
}
const refreshLabel = isRefreshing
? t("workspace.git.diff.refreshing")
: t("workspace.git.diff.refresh");
const refreshIcon = useMemo(
() =>
isRefreshing ? (
@@ -611,6 +517,9 @@ export function DiffOptionsMenu({
[isRefreshing],
);
const showChangesTab = host === "explorer" && !isMobile;
const showLayout = canUseSplitLayout && !changesTabOpen;
return (
<DropdownMenu>
<Tooltip delayDuration={300}>
@@ -618,10 +527,10 @@ export function DiffOptionsMenu({
<DropdownMenuTrigger
accessibilityRole="button"
accessibilityLabel={optionsLabel}
testID={`${testIDPrefix}-options-menu`}
style={overflowToggleStyle ?? defaultToggleStyle}
testID="changes-options-menu"
style={overflowToggleStyle}
>
<ThemedChevronDown
<ThemedMoreHorizontal
size={isMobile ? 18 : 14}
uniProps={foregroundMutedIconColorMapping}
/>
@@ -631,11 +540,40 @@ export function DiffOptionsMenu({
<Text style={styles.tooltipText}>{optionsLabel}</Text>
</TooltipContent>
</Tooltip>
<DropdownMenuContent align="end" width={240} testID={`${testIDPrefix}-options-menu-content`}>
<DropdownMenuContent align="end" width={240} testID="changes-options-menu-content">
{hasFiles ? (
<DropdownMenuItem
leading={allFilesCollapsed ? DIFF_OPTIONS_EXPAND_ICON : DIFF_OPTIONS_COLLAPSE_ICON}
testID="changes-toggle-collapse-all"
onSelect={allFilesCollapsed ? onExpandAll : onCollapseAll}
>
{collapseLabel}
</DropdownMenuItem>
) : null}
{showChangesTab ? (
<DropdownMenuItem
leading={DIFF_OPTIONS_CHANGES_TAB_ICON}
testID="changes-open-tab"
onSelect={onToggleChangesTab}
>
{changesTabLabel}
</DropdownMenuItem>
) : null}
{hasFiles || showChangesTab ? <DropdownMenuSeparator /> : null}
{showLayout ? (
<DropdownMenuItem
leading={DIFF_OPTIONS_SPLIT_ICON}
selected={layout === "split"}
testID="changes-toggle-layout"
onSelect={onToggleLayout}
>
{t("workspace.git.diff.split")}
</DropdownMenuItem>
) : null}
<DropdownMenuItem
leading={DIFF_OPTIONS_WHITESPACE_ICON}
selected={hideWhitespace}
testID={`${testIDPrefix}-toggle-whitespace`}
testID="changes-toggle-whitespace"
onSelect={onToggleHideWhitespace}
>
{whitespaceLabel}
@@ -643,18 +581,18 @@ export function DiffOptionsMenu({
<DropdownMenuItem
leading={DIFF_OPTIONS_WRAP_ICON}
selected={wrapLines}
testID={`${testIDPrefix}-toggle-wrap-lines`}
testID="changes-toggle-wrap-lines"
onSelect={onToggleWrapLines}
>
{wrapLinesLabel}
</DropdownMenuItem>
{refreshSupported && onRefresh ? (
{refreshSupported ? (
<>
<DropdownMenuSeparator />
<DropdownMenuItem
leading={refreshIcon}
disabled={isRefreshing}
testID={`${testIDPrefix}-refresh`}
testID="changes-refresh"
onSelect={onRefresh}
>
{refreshLabel}
@@ -1188,11 +1126,6 @@ export function ChangesSurface({
}, [changesPreferences.layout, updateChangesPreferences]);
const codeFontSize = appSettings.codeFontSize;
const layoutToggleStyle = useMemo(
() => buildToggleButtonStyle(false, styles.toolbarIconButton),
[],
);
const treeToggleStyle = useMemo(
() => buildToggleButtonStyle(desktopTreeVisible, styles.toolbarIconButton),
[desktopTreeVisible],
@@ -1419,6 +1352,10 @@ export function ChangesSurface({
);
const hasChanges = files.length > 0;
const selectedDiffStat = useMemo(
() => computeSelectedDiffStat(files, isDiffLoading),
[files, isDiffLoading],
);
const allFilesCollapsed =
hasChanges && files.every((file) => collapsedFilePaths.includes(file.path));
const handleCollapseAllFiles = useCallback(
@@ -1506,7 +1443,6 @@ export function ChangesSurface({
cwd={cwd}
desktopTreeVisible={desktopTreeVisible}
diffMode={diffMode}
forge={forge}
gitActions={gitActions}
hasFiles={hasChanges}
hideWhitespace={changesPreferences.hideWhitespace}
@@ -1514,9 +1450,9 @@ export function ChangesSurface({
isMobile={isMobile}
isRefreshing={isRefreshing}
layout={changesPreferences.layout}
layoutToggleStyle={layoutToggleStyle}
overflowToggleStyle={overflowToggleStyle}
refreshSupported={refreshSupported}
selectedDiffStat={selectedDiffStat}
serverId={serverId}
treeToggleStyle={treeToggleStyle}
workspaceId={workspaceId}
@@ -1555,11 +1491,6 @@ const styles = StyleSheet.create((theme) => ({
minHeight: 0,
},
changesToolbar: {
borderBottomWidth: 1,
borderBottomColor: theme.colors.border,
flexShrink: 0,
},
changesToolbarMainRow: {
minHeight: WORKSPACE_SECONDARY_HEADER_HEIGHT,
flexDirection: "row",
alignItems: "center",
@@ -1567,6 +1498,9 @@ const styles = StyleSheet.create((theme) => ({
gap: theme.spacing[2],
paddingLeft: theme.spacing[2],
paddingRight: theme.spacing[3],
borderBottomWidth: 1,
borderBottomColor: theme.colors.border,
flexShrink: 0,
},
changesToolbarIdentity: {
minWidth: 0,
@@ -1582,15 +1516,6 @@ const styles = StyleSheet.create((theme) => ({
gap: theme.spacing[1],
flexShrink: 0,
},
mobileGitActionsRow: {
minHeight: WORKSPACE_SECONDARY_HEADER_HEIGHT,
flexDirection: "row",
alignItems: "center",
justifyContent: "flex-end",
paddingHorizontal: theme.spacing[3],
borderTopWidth: 1,
borderTopColor: theme.colors.border,
},
diffModeTrigger: {
flexDirection: "row",
alignItems: "center",