Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions apps/desktop/src/renderer/lib/clickPolicy/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,19 @@ export { LinkHoverHint } from "./components/LinkHoverHint";
export { ShadowClickHint } from "./components/ShadowClickHint";
export { buildHint, UNBOUND_HINT } from "./hint";
export { modifierLabel } from "./modifierLabel";
export {
buildChangesSidebarFileHint,
type ChangesSidebarFileIntent,
resolveChangesSidebarFileIntent,
tierForChangesSidebarFileIntent,
} from "./policies/changesSidebarFilePolicy";
export {
type FolderIntent,
folderIntentFor,
folderIntentLabel,
} from "./policies/folderPolicy";
export type { ClickPolicy } from "./policies/policy";
export { useChangesSidebarFilePolicy } from "./policies/useChangesSidebarFilePolicy";
export { useInlineFilePolicy } from "./policies/useInlineFilePolicy";
export { useInlineUrlPolicy } from "./policies/useInlineUrlPolicy";
export { useSidebarFilePolicy } from "./policies/useSidebarFilePolicy";
Expand All @@ -29,4 +36,5 @@ export type {
Surface,
TierMode,
} from "./types";
export { usePierreChangesSidebarRowClickPolicy } from "./usePierreChangesSidebarRowClickPolicy";
export { usePierreRowClickPolicy } from "./usePierreRowClickPolicy";
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
import { describe, expect, it } from "bun:test";
import type { LinkTierMap } from "../types";
import {
resolveChangesSidebarFileIntent,
tierForChangesSidebarFileIntent,
} from "./changesSidebarFilePolicy";

const map: LinkTierMap = {
plain: "pane",
shift: "newTab",
meta: "pane",
metaShift: "external",
};

describe("changes sidebar file policy", () => {
it("keeps plain click on the diff", () => {
expect(
resolveChangesSidebarFileIntent(map, {
metaKey: false,
ctrlKey: false,
shiftKey: false,
}),
).toBe("diff");
});

it("maps shift-click through the settings map", () => {
expect(
resolveChangesSidebarFileIntent(map, {
metaKey: false,
ctrlKey: false,
shiftKey: true,
}),
).toBe("diffNewTab");
});

it("maps cmd/ctrl-click to the file when the settings action is pane", () => {
expect(
resolveChangesSidebarFileIntent(map, {
metaKey: true,
ctrlKey: false,
shiftKey: false,
}),
).toBe("file");
expect(
resolveChangesSidebarFileIntent(map, {
metaKey: false,
ctrlKey: true,
shiftKey: false,
}),
).toBe("file");
});

it("maps cmd/ctrl-shift-click to the external editor", () => {
expect(
resolveChangesSidebarFileIntent(map, {
metaKey: true,
ctrlKey: false,
shiftKey: true,
}),
).toBe("external");
expect(
resolveChangesSidebarFileIntent(map, {
metaKey: false,
ctrlKey: true,
shiftKey: true,
}),
).toBe("external");
});

it("returns null for unbound tiers", () => {
expect(
resolveChangesSidebarFileIntent(
{ ...map, meta: null },
{
metaKey: true,
ctrlKey: false,
shiftKey: false,
},
),
).toBeNull();
});

it("finds shortcuts for menu items from the same map", () => {
expect(tierForChangesSidebarFileIntent(map, "diffNewTab")).toBe("shift");
expect(tierForChangesSidebarFileIntent(map, "file")).toBe("meta");
expect(tierForChangesSidebarFileIntent(map, "external")).toBe("metaShift");
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
import { modifierLabel } from "../modifierLabel";
import { tierFor } from "../tiers";
import type {
LinkAction,
LinkTier,
LinkTierMap,
ModifierEvent,
} from "../types";

export type ChangesSidebarFileIntent =
| "diff"
| "diffNewTab"
| "file"
| "external";

const MODIFIER_TIERS: LinkTier[] = ["shift", "meta", "metaShift"];

function intentFor(
tier: LinkTier,
action: LinkAction | null,
): ChangesSidebarFileIntent | null {
if (action === null) return null;
if (action === "external") return "external";
if (action === "newTab") return "diffNewTab";
return tier === "plain" ? "diff" : "file";
}

function shortIntentLabel(intent: ChangesSidebarFileIntent): string {
if (intent === "diff") return "diff";
if (intent === "diffNewTab") return "diff in new tab";
if (intent === "file") return "open file";
return "editor";
}

export function resolveChangesSidebarFileIntent(
map: LinkTierMap,
event: ModifierEvent,
): ChangesSidebarFileIntent | null {
const tier = tierFor(event, "4-tier");
return intentFor(tier, map[tier]);
}

export function tierForChangesSidebarFileIntent(
map: LinkTierMap,
intent: ChangesSidebarFileIntent,
): LinkTier | null {
for (const tier of MODIFIER_TIERS) {
if (intentFor(tier, map[tier]) === intent) return tier;
}
return null;
}

export function buildChangesSidebarFileHint(map: LinkTierMap): string {
return MODIFIER_TIERS.flatMap((tier) => {
const intent = intentFor(tier, map[tier]);
if (intent === null) return [];
return `${modifierLabel(tier)}: ${shortIntentLabel(intent)}`;
}).join(" · ");
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import { useCallback, useMemo } from "react";
import {
buildChangesSidebarFileHint,
type ChangesSidebarFileIntent,
resolveChangesSidebarFileIntent,
tierForChangesSidebarFileIntent,
} from "./changesSidebarFilePolicy";
import { useSidebarFilePolicy } from "./useSidebarFilePolicy";

export function useChangesSidebarFilePolicy() {
const policy = useSidebarFilePolicy();

const getIntent = useCallback(
(event: Parameters<typeof resolveChangesSidebarFileIntent>[1]) =>
resolveChangesSidebarFileIntent(policy.map, event),
[policy.map],
);
const tierForIntent = useCallback(
(intent: ChangesSidebarFileIntent) =>
tierForChangesSidebarFileIntent(policy.map, intent),
[policy.map],
);
const hint = useMemo(
() => buildChangesSidebarFileHint(policy.map),
[policy.map],
);

return { ...policy, getIntent, tierForIntent, hint };
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
import { useCallback } from "react";
import type { ChangesSidebarFileIntent } from "./policies/changesSidebarFilePolicy";
import { folderIntentFor } from "./policies/folderPolicy";
import type { ModifierEvent } from "./types";

interface UsePierreChangesSidebarRowClickPolicyOptions {
getFileIntent: (event: ModifierEvent) => ChangesSidebarFileIntent | null;
onSelectDiff: (relativePath: string, openInNewTab?: boolean) => void;
onOpenFile: (relativePath: string, openInNewTab?: boolean) => void;
openInExternalEditor: (relativePath: string) => void;
}

interface UsePierreChangesSidebarRowClickPolicyResult {
onClickCapture: (e: React.MouseEvent<HTMLDivElement>) => void;
findFileRow: (e: React.MouseEvent) => HTMLElement | null;
}

export function usePierreChangesSidebarRowClickPolicy({
getFileIntent,
onSelectDiff,
onOpenFile,
openInExternalEditor,
}: UsePierreChangesSidebarRowClickPolicyOptions): UsePierreChangesSidebarRowClickPolicyResult {
const findRow = useCallback((e: React.MouseEvent): HTMLElement | null => {
const path = e.nativeEvent.composedPath();
for (const node of path) {
if (!(node instanceof HTMLElement)) continue;
if (node.getAttribute("data-item-path")) return node;
}
return null;
}, []);

const findFileRow = useCallback(
(e: React.MouseEvent): HTMLElement | null => {
const row = findRow(e);
const itemPath = row?.getAttribute("data-item-path");
if (!row || !itemPath || itemPath.endsWith("/")) return null;
return row;
},
[findRow],
);

const onClickCapture = useCallback(
(e: React.MouseEvent<HTMLDivElement>) => {
const treePath = findRow(e)?.getAttribute("data-item-path");
if (!treePath) return;
const trimmed = treePath.endsWith("/") ? treePath.slice(0, -1) : treePath;

if (treePath.endsWith("/")) {
const intent = folderIntentFor(e);
if (intent === null) return;
e.preventDefault();
e.stopPropagation();
if (intent === "external") openInExternalEditor(trimmed);
return;
}

const intent = getFileIntent(e);
if (intent === null) return;

e.preventDefault();
e.stopPropagation();

if (intent === "external") openInExternalEditor(trimmed);
else if (intent === "file") onOpenFile(trimmed, false);
else if (intent === "diffNewTab") onSelectDiff(trimmed, true);
else if (intent === "diff") onSelectDiff(trimmed, false);
Comment on lines +61 to +67
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Silent click consumption for unbound file tiers

For file rows, e.preventDefault() and e.stopPropagation() are called unconditionally before getFileIntent(e) is evaluated. When getFileIntent returns null (unbound tier), the click event is fully consumed but no action is taken. In contrast, the folder branch does the opposite — it returns early without consuming the event when folderIntentFor returns null, allowing the tree to handle expansion/collapse normally. A user who explicitly unbinds a modifier tier for files will get a silent no-op with no visual feedback, while the same situation on a folder correctly falls through to the tree.

Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/lib/clickPolicy/usePierreChangesSidebarRowClickPolicy.ts
Line: 58-65

Comment:
**Silent click consumption for unbound file tiers**

For file rows, `e.preventDefault()` and `e.stopPropagation()` are called unconditionally _before_ `getFileIntent(e)` is evaluated. When `getFileIntent` returns `null` (unbound tier), the click event is fully consumed but no action is taken. In contrast, the folder branch does the opposite — it returns early without consuming the event when `folderIntentFor` returns null, allowing the tree to handle expansion/collapse normally. A user who explicitly unbinds a modifier tier for files will get a silent no-op with no visual feedback, while the same situation on a folder correctly falls through to the tree.

How can I resolve this? If you propose a fix, please make it concise.

},
[findRow, getFileIntent, onSelectDiff, onOpenFile, openInExternalEditor],
);

return { onClickCapture, findFileRow };
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ import { Undo2 } from "lucide-react";
import { memo, useCallback, useEffect, useMemo, useRef, useState } from "react";
import {
ShadowClickHint,
usePierreRowClickPolicy,
useSidebarFilePolicy,
useChangesSidebarFilePolicy,
usePierreChangesSidebarRowClickPolicy,
} from "renderer/lib/clickPolicy";
import { useFallthroughIcons } from "renderer/lib/fileIcons";
import {
Expand All @@ -29,7 +29,10 @@ import {
import { DiscardConfirmDialog } from "renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/DiscardConfirmDialog";
import { PierreRowContextMenu } from "renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PierreRowContextMenu";
import type { ChangesetFile } from "renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useChangeset";
import { toRelativeWorkspacePath } from "shared/absolute-paths";
import {
toAbsoluteWorkspacePath,
toRelativeWorkspacePath,
} from "shared/absolute-paths";
import type { FoldSignal } from "../../ChangesFileList";
import { FileRowContextMenuItems } from "./components/FileRowContextMenuItems";
import { FolderContextMenuItems } from "./components/FolderContextMenuItems";
Expand Down Expand Up @@ -75,7 +78,7 @@ interface ChangesTreeViewProps {
* - `renderContextMenu`: file-row actions matching `FileRow`; folder-row
* actions (open in editor, copy path)
* - hover actions overlay (Discard on unstaged + more-actions ⌄ dropdown)
* - `usePierreRowClickPolicy` for settings-driven click routing
* - `useChangesSidebarFilePolicy` for settings-driven click routing
* - selection echo: when the diff pane's file is in this section, focus it
*
* The discard confirm dialog lives here, not in the per-row menus: Pierre
Expand Down Expand Up @@ -212,15 +215,21 @@ export const ChangesTreeView = memo(function ChangesTreeView({
return text ? { text } : null;
};

const filePolicy = useSidebarFilePolicy();
const { onClickCapture, findFileRow } = usePierreRowClickPolicy({
filePolicy,
onSelectFile: (rel, openInNewTab) => {
lastUserSelectRef.current = rel;
onSelectFile?.(rel, openInNewTab);
const filePolicy = useChangesSidebarFilePolicy();
const { onClickCapture, findFileRow } = usePierreChangesSidebarRowClickPolicy(
{
getFileIntent: filePolicy.getIntent,
onSelectDiff: (rel, openInNewTab) => {
lastUserSelectRef.current = rel;
onSelectFile?.(rel, openInNewTab);
},
onOpenFile: (rel, openInNewTab) => {
if (!worktreePath) return;
onOpenFile?.(toAbsoluteWorkspacePath(worktreePath, rel), openInNewTab);
},
openInExternalEditor: (rel) => onOpenInEditor?.(rel),
},
openInExternalEditor: (rel) => onOpenInEditor?.(rel),
});
);

// Hoisted so the dialog outlives the menu/hover overlay that triggers it.
const [discardTarget, setDiscardTarget] = useState<ChangesetFile | null>(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,10 @@ import {
Trash2,
Undo2,
} from "lucide-react";
import { modifierLabel, useSidebarFilePolicy } from "renderer/lib/clickPolicy";
import {
modifierLabel,
useChangesSidebarFilePolicy,
} from "renderer/lib/clickPolicy";
import { PathActionsMenuItems } from "renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/PathActionsMenuItems";
import type { ChangesetFile } from "renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useChangeset";
import { toAbsoluteWorkspacePath } from "shared/absolute-paths";
Expand Down Expand Up @@ -52,9 +55,10 @@ export function FileRowContextMenuItems({
const canDiscard = sectionKind === "unstaged";
const isDeleteAction = file.status === "untracked" || file.status === "added";

const policy = useSidebarFilePolicy();
const newTabTier = policy.tierForAction("newTab");
const externalTier = policy.tierForAction("external");
const policy = useChangesSidebarFilePolicy();
const diffNewTabTier = policy.tierForIntent("diffNewTab");
const fileTier = policy.tierForIntent("file");
const externalTier = policy.tierForIntent("external");

return (
<>
Expand All @@ -65,9 +69,9 @@ export function FileRowContextMenuItems({
<DropdownMenuItem onSelect={() => onSelectFile?.(file.path, true)}>
<SquarePlus />
Open Diff in New Tab
{newTabTier && (
{diffNewTabTier && (
<DropdownMenuShortcut>
{modifierLabel(newTabTier)}
{modifierLabel(diffNewTabTier)}
</DropdownMenuShortcut>
)}
</DropdownMenuItem>
Expand All @@ -77,6 +81,9 @@ export function FileRowContextMenuItems({
>
<FileText />
Open File
{fileTier && (
<DropdownMenuShortcut>{modifierLabel(fileTier)}</DropdownMenuShortcut>
)}
</DropdownMenuItem>
<DropdownMenuItem
onSelect={() => absolutePath && onOpenFile?.(absolutePath, true)}
Expand Down
Loading
Loading