Skip to content
Open
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
31 changes: 31 additions & 0 deletions packages/core/src/git-interaction/branchName.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import {
sanitizeBranchName,
suggestBranchName,
validateBranchName,
validateBranchPrefix,
} from "./branchName";

describe("sanitizeBranchName", () => {
Expand Down Expand Up @@ -112,4 +113,34 @@ describe("suggestBranchName", () => {
]),
).toBe("posthog-code/fix-bug-4");
});

it.each([
{ existing: [] as string[], expected: "team/fix-bug" },
{ existing: ["team/fix-bug"], expected: "team/fix-bug-2" },
])(
"uses a custom prefix (existing $existing -> $expected)",
({ existing, expected }) => {
expect(suggestBranchName("Fix bug", "abc", existing, "team/")).toBe(
expected,
);
},
);
});

describe("validateBranchPrefix", () => {
it.each([
{ prefix: "", expected: null },
{ prefix: "team/", expected: null },
{ prefix: "posthog-code/", expected: null },
{ prefix: "-x/", expected: "Branch prefix cannot start with a dash." },
])("returns $expected for prefix $prefix", ({ prefix, expected }) => {
expect(validateBranchPrefix(prefix)).toBe(expected);
});

it.each(["my team/", "../", "~/"])(
"rejects an invalid prefix (%j)",
(prefix) => {
expect(validateBranchPrefix(prefix)).not.toBeNull();
},
);
});
Comment on lines 114 to 146

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 Prefer parameterised tests for validateBranchPrefix

The five separate it blocks under validateBranchPrefix all call the same function with different inputs; this is exactly the pattern the team prefers to express as it.each. A failure in the current form only tells you "the test named 'rejects spaces' failed" rather than which input/expected pair broke — and it takes more prose to add a new case. The same applies to the three new it blocks in deriveBranchName.test.ts ("uses a custom prefix", "supports an empty prefix", "applies a custom prefix to the task-ID fallback"), and to the two-assertion "supports a custom prefix" test for suggestBranchName which packs two independent scenarios into one case.

Context Used: Do not attempt to comment on incorrect alphabetica... (source)

Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/core/src/git-interaction/branchName.test.ts
Line: 114-150

Comment:
**Prefer parameterised tests for `validateBranchPrefix`**

The five separate `it` blocks under `validateBranchPrefix` all call the same function with different inputs; this is exactly the pattern the team prefers to express as `it.each`. A failure in the current form only tells you "the test named 'rejects spaces' failed" rather than which input/expected pair broke — and it takes more prose to add a new case. The same applies to the three new `it` blocks in `deriveBranchName.test.ts` ("uses a custom prefix", "supports an empty prefix", "applies a custom prefix to the task-ID fallback"), and to the two-assertion `"supports a custom prefix"` test for `suggestBranchName` which packs two independent scenarios into one case.

**Context Used:** Do not attempt to comment on incorrect alphabetica... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))

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

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

25 changes: 21 additions & 4 deletions packages/core/src/git-interaction/branchName.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,23 @@ export function validateBranchName(name: string): string | null {
return null;
}

export function deriveBranchName(title: string, fallbackId: string): string {
// A user-configured branch prefix is prepended to generated branch names, so it
// must form a valid leading segment. Empty means "no prefix". We reject a
// leading dash defensively (git could read it as a flag) and otherwise validate
// it as the start of a real branch name.
export function validateBranchPrefix(prefix: string): string | null {
if (prefix === "") return null;
if (prefix.startsWith("-")) {
return "Branch prefix cannot start with a dash.";
}
return validateBranchName(`${prefix}example`);
}

export function deriveBranchName(
title: string,
fallbackId: string,
prefix: string = BRANCH_PREFIX,
): string {
const slug = title
.toLowerCase()
.trim()
Expand All @@ -64,16 +80,17 @@ export function deriveBranchName(title: string, fallbackId: string): string {
.slice(0, 60)
.replace(/-$/, "");

if (!slug) return `${BRANCH_PREFIX}task-${fallbackId}`;
return `${BRANCH_PREFIX}${slug}`;
if (!slug) return `${prefix}task-${fallbackId}`;
return `${prefix}${slug}`;
}

export function suggestBranchName(
title: string,
fallbackId: string,
existingBranches: string[],
prefix: string = BRANCH_PREFIX,
): string {
const base = deriveBranchName(title, fallbackId);
const base = deriveBranchName(title, fallbackId, prefix);

if (!existingBranches.includes(base)) return base;

Expand Down
11 changes: 11 additions & 0 deletions packages/core/src/git-interaction/deriveBranchName.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,4 +48,15 @@ describe("deriveBranchName", () => {
"posthog-code/task-abc123",
);
});

it.each([
{ title: "Fix login bug", prefix: "team/", expected: "team/fix-login-bug" },
{ title: "Fix login bug", prefix: "", expected: "fix-login-bug" },
{ title: "", prefix: "team/", expected: "team/task-abc123" },
])(
"applies a custom prefix ($title, $prefix -> $expected)",
({ title, prefix, expected }) => {
expect(deriveBranchName(title, "abc123", prefix)).toBe(expected);
},
);
});
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {
} from "@posthog/core/git-interaction/branchName";
import type { Task } from "@posthog/shared/domain-types";
import type { QueryClient } from "@tanstack/react-query";
import { useSettingsStore } from "../../settings/settingsStore";
import type { GitCacheKeyProvider } from "../gitCacheProvider";

export function getSuggestedBranchName(
Expand All @@ -24,12 +25,14 @@ export function getSuggestedBranchName(
? String(task.task_number)
: (task?.slug ?? taskId);

if (!repoPath) return deriveBranchName(task?.title ?? "", fallbackId);
const prefix = useSettingsStore.getState().branchPrefix;

if (!repoPath) return deriveBranchName(task?.title ?? "", fallbackId, prefix);

const cached =
queryClient.getQueryData<string[]>(
provider.gitQueryKey("getAllBranches", { directoryPath: repoPath }),
) ?? [];

return suggestBranchName(task?.title ?? "", fallbackId, cached);
return suggestBranchName(task?.title ?? "", fallbackId, cached, prefix);
}
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { validateBranchPrefix } from "@posthog/core/git-interaction/branchName";
import {
buildTaskMap,
groupWorktrees,
Expand All @@ -7,7 +8,8 @@ import { deleteWorktree as runDeleteWorktree } from "@posthog/core/settings/work
import { useHostTRPC, useHostTRPCClient } from "@posthog/host-router/react";
import { Flex, Switch, Text, TextField } from "@radix-ui/themes";
import { useQueries, useQueryClient } from "@tanstack/react-query";
import { useCallback, useMemo, useState } from "react";
import { useCallback, useEffect, useMemo, useState } from "react";
import { useDebounce } from "../../../../primitives/hooks/useDebounce";
import { toast } from "../../../../primitives/toast";
import { logger } from "../../../../shell/logger";
import { useFolders } from "../../../folders/useFolders";
Expand All @@ -16,6 +18,7 @@ import { useDeleteTask } from "../../../tasks/useTaskCrudMutations";
import { useTasks } from "../../../tasks/useTasks";
import { WORKSPACE_QUERY_KEY } from "../../../workspace/identifiers";
import { SettingRow } from "../../SettingRow";
import { useSettingsStore } from "../../settingsStore";
import { WorktreeGroupSection } from "./WorktreeGroupSection";

const log = logger.scope("worktrees-settings");
Expand All @@ -33,6 +36,21 @@ export function WorktreesSettings() {
const { folders } = useFolders();
const { data: tasks } = useTasks();

const branchPrefix = useSettingsStore((s) => s.branchPrefix);
const setBranchPrefix = useSettingsStore((s) => s.setBranchPrefix);
const [draftBranchPrefix, setDraftBranchPrefix] = useState(branchPrefix);
const debouncedBranchPrefix = useDebounce(draftBranchPrefix, 500);
const branchPrefixError = validateBranchPrefix(draftBranchPrefix);
useEffect(() => {
setDraftBranchPrefix(branchPrefix);
}, [branchPrefix]);
useEffect(() => {
if (debouncedBranchPrefix === branchPrefix) return;
// Don't persist an invalid prefix; the inline error prompts a fix.
if (validateBranchPrefix(debouncedBranchPrefix) !== null) return;
setBranchPrefix(debouncedBranchPrefix);
}, [debouncedBranchPrefix, branchPrefix, setBranchPrefix]);

const worktreeQueries = useQueries({
queries: folders.map((folder) => ({
queryKey: trpc.workspace.listGitWorktrees.queryKey({
Expand Down Expand Up @@ -133,6 +151,30 @@ export function WorktreesSettings() {
return (
<Flex direction="column" gap="5">
<Flex direction="column">
<SettingRow
label="Branch name prefix"
description="Prefix for branches PostHog Code creates for new tasks. Leave empty for no prefix."
>
<Flex direction="column" align="end" gap="1">
<TextField.Root
value={draftBranchPrefix}
onChange={(e) => setDraftBranchPrefix(e.target.value)}
placeholder="posthog-code/"
size="1"
className="min-w-[200px]"
color={branchPrefixError ? "red" : undefined}
/>
{branchPrefixError ? (
<Text color="red" className="text-[12px]">
{branchPrefixError}
</Text>
) : (
<Text color="gray" className="text-[12px]">
Example: posthog-code/
</Text>
)}
</Flex>
</SettingRow>
<SettingRow
label="Automatically suspend stale worktrees"
description="Suspend stale worktrees to save disk space. Suspended worktrees can be restored at any time. Only disable if you prefer to manage worktrees manually."
Expand Down
17 changes: 16 additions & 1 deletion packages/ui/src/features/settings/settingsStore.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
import type { ExecutionMode, WorkspaceMode } from "@posthog/shared";
import {
BRANCH_PREFIX,
type ExecutionMode,
type WorkspaceMode,
} from "@posthog/shared";
import {
COLLAPSE_MODE_DEFAULT,
type CollapseMode,
Expand Down Expand Up @@ -109,6 +113,10 @@ interface SettingsStore {
diffOpenMode: DiffOpenMode;
setDiffOpenMode: (mode: DiffOpenMode) => void;

// Git / branches
branchPrefix: string;
setBranchPrefix: (value: string) => void;

// System / power / permissions
allowBypassPermissions: boolean;
preventSleepWhileRunning: boolean;
Expand Down Expand Up @@ -221,6 +229,10 @@ export const useSettingsStore = create<SettingsStore>()(
diffOpenMode: "auto",
setDiffOpenMode: (mode) => set({ diffOpenMode: mode }),

// Git / branches
branchPrefix: BRANCH_PREFIX,
setBranchPrefix: (value) => set({ branchPrefix: value }),

// System / power / permissions
allowBypassPermissions: false,
preventSleepWhileRunning: false,
Expand Down Expand Up @@ -317,6 +329,9 @@ export const useSettingsStore = create<SettingsStore>()(
// Diff viewer
diffOpenMode: state.diffOpenMode,

// Git / branches
branchPrefix: state.branchPrefix,

// System / power / permissions
allowBypassPermissions: state.allowBypassPermissions,
preventSleepWhileRunning: state.preventSleepWhileRunning,
Expand Down