-
Notifications
You must be signed in to change notification settings - Fork 37
feat(task-input): pin recently-used repos atop the cloud repo picker #2630
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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, | ||||||
|
|
@@ -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"; | ||||||
|
|
@@ -41,6 +57,7 @@ export function GitHubRepoPicker({ | |||||
| value, | ||||||
| onChange, | ||||||
| repositories, | ||||||
| recentRepositories, | ||||||
| isLoading, | ||||||
| placeholder = "Select repository...", | ||||||
| disabled = false, | ||||||
|
|
@@ -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)), | ||||||
| ]; | ||||||
| }, [pinnedRecentRepositories, pinnedRecentSet, repositories]); | ||||||
| const filteredRepositoryCount = useMemo(() => { | ||||||
| if (!trimmedSearchQuery) { | ||||||
| return repositories.length; | ||||||
|
|
@@ -136,7 +182,7 @@ export function GitHubRepoPicker({ | |||||
|
|
||||||
| return ( | ||||||
| <Combobox | ||||||
| items={repositories} | ||||||
| items={displayRepositories} | ||||||
| limit={visibleLimit} | ||||||
| value={value} | ||||||
| onValueChange={(v) => { | ||||||
|
|
@@ -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); | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Prompt To Fix With AIThis 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 || | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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; | ||||||||||||||
|
|
@@ -110,6 +113,8 @@ export function TaskInput({ | |||||||||||||
| setLastUsedAdapter, | ||||||||||||||
| lastUsedCloudRepository, | ||||||||||||||
| setLastUsedCloudRepository, | ||||||||||||||
| recentCloudRepositories, | ||||||||||||||
| addRecentCloudRepository, | ||||||||||||||
| allowBypassPermissions, | ||||||||||||||
| setLastUsedEnvironment, | ||||||||||||||
| getLastUsedEnvironment, | ||||||||||||||
|
|
@@ -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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Prompt To Fix With AIThis 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"; | ||||||||||||||
|
|
@@ -320,8 +335,9 @@ export function TaskInput({ | |||||||||||||
| const normalizedRepo = repo.toLowerCase(); | ||||||||||||||
| setSelectedRepository(normalizedRepo); | ||||||||||||||
| setLastUsedCloudRepository(normalizedRepo); | ||||||||||||||
| addRecentCloudRepository(normalizedRepo); | ||||||||||||||
| }, | ||||||||||||||
| [setLastUsedCloudRepository], | ||||||||||||||
| [setLastUsedCloudRepository, addRecentCloudRepository], | ||||||||||||||
| ); | ||||||||||||||
|
|
||||||||||||||
| useEffect(() => { | ||||||||||||||
|
|
@@ -697,6 +713,7 @@ export function TaskInput({ | |||||||||||||
| ? visibleCloudRepositories | ||||||||||||||
| : repositories | ||||||||||||||
| } | ||||||||||||||
| recentRepositories={pinnedRecentRepositories} | ||||||||||||||
| isLoading={ | ||||||||||||||
| isLoadingRepos || | ||||||||||||||
| (isCloudRepoPickerOpen && cloudRepositoriesLoading) | ||||||||||||||
|
|
||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pinnedRecentSetcontains lowercased strings (from the store), butrepositoriesitems may carry their original API casing. The case-sensitiveSet.hascheck means a mixed-case repo like"PostHog/code"passes the filter and ends up indisplayRepositoriesa second time — once as the pinned lowercase entry and once from the unfiltered tail ofrepositories. Normalising the comparison here closes the gap.Prompt To Fix With AI