Skip to content
Draft
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
88 changes: 80 additions & 8 deletions packages/ui/src/features/folder-picker/GitHubRepoPicker.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
import { ArrowClockwise, GithubLogo } from "@phosphor-icons/react";
import {
ArrowClockwise,
ClockCounterClockwise,
GithubLogo,
} from "@phosphor-icons/react";
import {
Button,
Combobox,
Expand All @@ -12,14 +16,26 @@ import {
} from "@posthog/quill";
import { Tooltip } from "@posthog/ui/primitives/Tooltip";
import { defaultFilter } from "cmdk";
import { type RefObject, useEffect, useMemo, useRef, useState } from "react";
import {
Fragment,
type RefObject,
useEffect,
useMemo,
useRef,
useState,
} from "react";

const COMBOBOX_INITIAL_LIMIT = 50;

interface GitHubRepoPickerProps {
value: string | null;
onChange: (repo: string | null) => void;
repositories: string[];
/**
* Recently-used repositories to pin above the full list, most-recent first.
* Only shown while there is no active search query.
*/
recentRepositories?: string[];
isLoading: boolean;
placeholder?: string;
size?: "1" | "2";
Expand All @@ -41,6 +57,7 @@ export function GitHubRepoPicker({
value,
onChange,
repositories,
recentRepositories,
isLoading,
placeholder = "Select repository...",
disabled = false,
Expand Down Expand Up @@ -70,6 +87,35 @@ export function GitHubRepoPicker({
const onlyRepo =
!remoteMode && repositories.length === 1 ? repositories[0] : null;
const trimmedSearchQuery = searchQuery.trim();
// Pin recently-used repos to the top, but only while idle: once the user
// starts searching they want matches, not their history.
const pinnedRecentRepositories = useMemo(() => {
if (trimmedSearchQuery || !recentRepositories?.length) {
return [] as string[];
}
const seen = new Set<string>();
const result: string[] = [];
for (const repo of recentRepositories) {
if (!seen.has(repo)) {
seen.add(repo);
result.push(repo);
}
}
return result;
}, [recentRepositories, trimmedSearchQuery]);
const pinnedRecentSet = useMemo(
() => new Set(pinnedRecentRepositories),
[pinnedRecentRepositories],
);
const displayRepositories = useMemo(() => {
if (pinnedRecentRepositories.length === 0) {
return repositories;
}
return [
...pinnedRecentRepositories,
...repositories.filter((repo) => !pinnedRecentSet.has(repo)),
];
Comment on lines +114 to +117

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.

P1 The pinnedRecentSet contains lowercased strings (from the store), but repositories items may carry their original API casing. The case-sensitive Set.has check means a mixed-case repo like "PostHog/code" passes the filter and ends up in displayRepositories a second time — once as the pinned lowercase entry and once from the unfiltered tail of repositories. Normalising the comparison here closes the gap.

Suggested change
return [
...pinnedRecentRepositories,
...repositories.filter((repo) => !pinnedRecentSet.has(repo)),
];
return [
...pinnedRecentRepositories,
...repositories.filter((repo) => !pinnedRecentSet.has(repo.toLowerCase())),
];
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/ui/src/features/folder-picker/GitHubRepoPicker.tsx
Line: 114-117

Comment:
The `pinnedRecentSet` contains lowercased strings (from the store), but `repositories` items may carry their original API casing. The case-sensitive `Set.has` check means a mixed-case repo like `"PostHog/code"` passes the filter and ends up in `displayRepositories` a second time — once as the pinned lowercase entry and once from the unfiltered tail of `repositories`. Normalising the comparison here closes the gap.

```suggestion
    return [
      ...pinnedRecentRepositories,
      ...repositories.filter((repo) => !pinnedRecentSet.has(repo.toLowerCase())),
    ];
```

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

}, [pinnedRecentRepositories, pinnedRecentSet, repositories]);
const filteredRepositoryCount = useMemo(() => {
if (!trimmedSearchQuery) {
return repositories.length;
Expand Down Expand Up @@ -136,7 +182,7 @@ export function GitHubRepoPicker({

return (
<Combobox
items={repositories}
items={displayRepositories}
limit={visibleLimit}
value={value}
onValueChange={(v) => {
Expand Down Expand Up @@ -215,11 +261,37 @@ export function GitHubRepoPicker({
: "No repositories found."}
</ComboboxEmpty>
<ComboboxList>
{(repo: string) => (
<ComboboxItem key={repo} value={repo}>
{repo}
</ComboboxItem>
)}
{(repo: string) => {
const isPinned = pinnedRecentSet.has(repo);

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.

P1 Likewise, isPinned is computed with a case-sensitive lookup against pinnedRecentSet. If a mixed-case repo reaches this render path, the clock icon and divider will not render correctly. Normalising here keeps the rendering consistent with the deduplication logic above.

Suggested change
const isPinned = pinnedRecentSet.has(repo);
const isPinned = pinnedRecentSet.has(repo.toLowerCase());
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/ui/src/features/folder-picker/GitHubRepoPicker.tsx
Line: 265

Comment:
Likewise, `isPinned` is computed with a case-sensitive lookup against `pinnedRecentSet`. If a mixed-case repo reaches this render path, the clock icon and divider will not render correctly. Normalising here keeps the rendering consistent with the deduplication logic above.

```suggestion
            const isPinned = pinnedRecentSet.has(repo.toLowerCase());
```

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

const isFirstPinned =
isPinned && repo === pinnedRecentRepositories[0];
const isLastPinned =
isPinned &&
repo ===
pinnedRecentRepositories[pinnedRecentRepositories.length - 1];
return (
<Fragment key={repo}>
{isFirstPinned ? (
<div className="px-2 pt-1 pb-0.5 font-medium text-muted-foreground text-xs">
Recent
</div>
) : null}
<ComboboxItem value={repo}>
{isPinned ? (
<ClockCounterClockwise
size={14}
weight="regular"
className="shrink-0 text-muted-foreground"
/>
) : null}
<span className="min-w-0 truncate">{repo}</span>
</ComboboxItem>
{isLastPinned ? (
<div className="my-1 border-border border-t" />
) : null}
</Fragment>
);
}}
</ComboboxList>

{(hasMore ||
Expand Down
45 changes: 44 additions & 1 deletion packages/ui/src/features/settings/settingsStore.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
import { registerRendererStateStorage } from "@posthog/ui/shell/rendererStorage";
import { beforeEach, describe, expect, it, vi } from "vitest";
import { useSettingsStore } from "./settingsStore";
import {
RECENT_CLOUD_REPOSITORIES_LIMIT,
useSettingsStore,
} from "./settingsStore";

const getItem = vi.fn();
const setItem = vi.fn();
Expand All @@ -20,6 +23,7 @@ describe("feature settingsStore cloud selections", () => {
useSettingsStore.setState({
allowBypassPermissions: false,
lastUsedCloudRepository: null,
recentCloudRepositories: [],
});
});

Expand Down Expand Up @@ -57,6 +61,45 @@ describe("feature settingsStore cloud selections", () => {
);
});

it("tracks recently used cloud repositories most-recent first", () => {
const { addRecentCloudRepository } = useSettingsStore.getState();

addRecentCloudRepository("posthog/posthog");
addRecentCloudRepository("posthog/posthog-js");
addRecentCloudRepository("PostHog/Code");

expect(useSettingsStore.getState().recentCloudRepositories).toEqual([
"posthog/code",
"posthog/posthog-js",
"posthog/posthog",
]);
});

it("dedupes and promotes a re-selected repository to the front", () => {
const { addRecentCloudRepository } = useSettingsStore.getState();

addRecentCloudRepository("posthog/posthog");
addRecentCloudRepository("posthog/posthog-js");
addRecentCloudRepository("posthog/posthog");

expect(useSettingsStore.getState().recentCloudRepositories).toEqual([
"posthog/posthog",
"posthog/posthog-js",
]);
});

it("caps the recent repositories list", () => {
const { addRecentCloudRepository } = useSettingsStore.getState();

for (let i = 0; i < RECENT_CLOUD_REPOSITORIES_LIMIT + 3; i++) {
addRecentCloudRepository(`posthog/repo-${i}`);
}

expect(useSettingsStore.getState().recentCloudRepositories).toHaveLength(
RECENT_CLOUD_REPOSITORIES_LIMIT,
);
});

it("rehydrates the unsafe mode toggle", async () => {
getItem.mockResolvedValue(
JSON.stringify({
Expand Down
16 changes: 16 additions & 0 deletions packages/ui/src/features/settings/settingsStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,9 @@ export interface HintState {
learned: boolean;
}

/** How many recently-used cloud repositories we remember for the picker. */
export const RECENT_CLOUD_REPOSITORIES_LIMIT = 5;

// ---------- Store shape ----------

interface SettingsStore {
Expand All @@ -60,6 +63,7 @@ interface SettingsStore {
lastUsedModel: string | null;
lastUsedReasoningEffort: string | null;
lastUsedCloudRepository: string | null;
recentCloudRepositories: string[];
lastUsedEnvironments: Record<string, string>;
defaultInitialTaskMode: DefaultInitialTaskMode;
lastUsedInitialTaskMode: ExecutionMode;
Expand All @@ -72,6 +76,7 @@ interface SettingsStore {
setLastUsedModel: (model: string) => void;
setLastUsedReasoningEffort: (effort: string) => void;
setLastUsedCloudRepository: (repo: string | null) => void;
addRecentCloudRepository: (repo: string) => void;
setLastUsedEnvironment: (
repoPath: string,
environmentId: string | null,
Expand Down Expand Up @@ -149,6 +154,7 @@ export const useSettingsStore = create<SettingsStore>()(
lastUsedModel: null,
lastUsedReasoningEffort: null,
lastUsedCloudRepository: null,
recentCloudRepositories: [],
lastUsedEnvironments: {},
defaultInitialTaskMode: "plan",
lastUsedInitialTaskMode: "plan",
Expand All @@ -164,6 +170,15 @@ export const useSettingsStore = create<SettingsStore>()(
set({ lastUsedReasoningEffort: effort }),
setLastUsedCloudRepository: (repo) =>
set({ lastUsedCloudRepository: repo }),
addRecentCloudRepository: (repo) =>
set((state) => {
const normalized = repo.toLowerCase();
const next = [
normalized,
...state.recentCloudRepositories.filter((r) => r !== normalized),
].slice(0, RECENT_CLOUD_REPOSITORIES_LIMIT);
return { recentCloudRepositories: next };
}),
setLastUsedEnvironment: (repoPath, environmentId) =>
set((state) => {
const next = { ...state.lastUsedEnvironments };
Expand Down Expand Up @@ -279,6 +294,7 @@ export const useSettingsStore = create<SettingsStore>()(
lastUsedModel: state.lastUsedModel,
lastUsedReasoningEffort: state.lastUsedReasoningEffort,
lastUsedCloudRepository: state.lastUsedCloudRepository,
recentCloudRepositories: state.recentCloudRepositories,
lastUsedEnvironments: state.lastUsedEnvironments,
defaultInitialTaskMode: state.defaultInitialTaskMode,
lastUsedInitialTaskMode: state.lastUsedInitialTaskMode,
Expand Down
19 changes: 18 additions & 1 deletion packages/ui/src/features/task-detail/components/TaskInput.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,9 @@ import { CloudGithubMissingNotice } from "./CloudGithubMissingNotice";
import { SuggestedTasksPanel } from "./SuggestedTasksPanel";
import { type WorkspaceMode, WorkspaceModeSelect } from "./WorkspaceModeSelect";

/** How many recently-used repos to pin above the full cloud repo list. */
const RECENT_CLOUD_REPOSITORIES_DISPLAY_COUNT = 3;

interface TaskInputProps {
sessionId?: string;
onTaskCreated?: (task: Task) => void;
Expand Down Expand Up @@ -110,6 +113,8 @@ export function TaskInput({
setLastUsedAdapter,
lastUsedCloudRepository,
setLastUsedCloudRepository,
recentCloudRepositories,
addRecentCloudRepository,
allowBypassPermissions,
setLastUsedEnvironment,
getLastUsedEnvironment,
Expand Down Expand Up @@ -207,6 +212,16 @@ export function TaskInput({
hasGithubIntegration,
} = useUserRepositoryIntegration();

// Surface the few most recently-used repos at the top of the picker, keeping
// only those still connected so we never pin a repo the user has lost access to.
const pinnedRecentRepositories = useMemo(() => {
if (repositories.length === 0) return [];
const connected = new Set(repositories);
return recentCloudRepositories
.filter((repo) => connected.has(repo))
Comment on lines +219 to +221

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.

P1 The connected set is built from repositories (whose strings come from the API via Object.keys(repositoryMap) without normalization), but recentCloudRepositories entries are always stored lowercase by addRecentCloudRepository. A case-sensitive Set.has() lookup will miss any repo whose API name contains uppercase letters (e.g., "PostHog/posthog"), silently keeping the Recent section empty even after the user has picked that repo. The existing normalizeRepoKey helper in repositories.ts exists precisely because API-returned names are not guaranteed lowercase.

Suggested change
const connected = new Set(repositories);
return recentCloudRepositories
.filter((repo) => connected.has(repo))
const connected = new Set(repositories.map((r) => r.toLowerCase()));
return recentCloudRepositories
.filter((repo) => connected.has(repo))
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/ui/src/features/task-detail/components/TaskInput.tsx
Line: 219-221

Comment:
The `connected` set is built from `repositories` (whose strings come from the API via `Object.keys(repositoryMap)` without normalization), but `recentCloudRepositories` entries are always stored lowercase by `addRecentCloudRepository`. A case-sensitive `Set.has()` lookup will miss any repo whose API name contains uppercase letters (e.g., `"PostHog/posthog"`), silently keeping the Recent section empty even after the user has picked that repo. The existing `normalizeRepoKey` helper in `repositories.ts` exists precisely because API-returned names are not guaranteed lowercase.

```suggestion
    const connected = new Set(repositories.map((r) => r.toLowerCase()));
    return recentCloudRepositories
      .filter((repo) => connected.has(repo))
```

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

.slice(0, RECENT_CLOUD_REPOSITORIES_DISPLAY_COUNT);
}, [recentCloudRepositories, repositories]);

const [workspaceMode, setWorkspaceModeState] = useState<WorkspaceMode>(() => {
if (initialCloudRepository) return "cloud";
return lastUsedWorkspaceMode || "local";
Expand Down Expand Up @@ -320,8 +335,9 @@ export function TaskInput({
const normalizedRepo = repo.toLowerCase();
setSelectedRepository(normalizedRepo);
setLastUsedCloudRepository(normalizedRepo);
addRecentCloudRepository(normalizedRepo);
},
[setLastUsedCloudRepository],
[setLastUsedCloudRepository, addRecentCloudRepository],
);

useEffect(() => {
Expand Down Expand Up @@ -697,6 +713,7 @@ export function TaskInput({
? visibleCloudRepositories
: repositories
}
recentRepositories={pinnedRecentRepositories}
isLoading={
isLoadingRepos ||
(isCloudRepoPickerOpen && cloudRepositoriesLoading)
Expand Down
Loading