fix: address review findings (#2575)

This commit is contained in:
Jinjing 2026-05-21 17:56:41 -07:00 committed by GitHub
parent 0153fcb1d1
commit 131aa3a08f
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 6187 additions and 100 deletions

View File

@ -4253,7 +4253,8 @@ function GHEditSection({
onLabelsChange,
onMutated,
assignees,
onUse
onUse,
layout = 'horizontal'
}: {
item: GitHubWorkItem
repoPath: string | null
@ -4269,6 +4270,10 @@ function GHEditSection({
onMutated: () => void
assignees: string[]
onUse: (item: GitHubWorkItem) => void
/** `'horizontal'` is the legacy strip rendered above the conversation; the
* `'sidebar'` layout matches the GitHub issue page's right rail with each
* metadata row stacked under a section heading. */
layout?: 'horizontal' | 'sidebar'
}): React.JSX.Element | null {
const [labelPopoverOpen, setLabelPopoverOpen] = useState(false)
const [assigneePopoverOpen, setAssigneePopoverOpen] = useState(false)
@ -4531,6 +4536,222 @@ function GHEditSection({
</svg>
)
if (layout === 'sidebar') {
return (
<aside className="flex flex-col gap-5 text-[13px]">
{/* State */}
<section>
<div className="mb-2 text-[11px] font-semibold uppercase tracking-[0.05em] text-muted-foreground">
Status
</div>
<Popover>
<PopoverTrigger asChild>
<button
type="button"
className={cn(
'inline-flex w-full items-center justify-between gap-2 rounded-md border px-2.5 py-1.5 text-[12px] font-medium transition hover:brightness-125 hover:ring-1 hover:ring-white/10',
getStateTone({ ...item, state: localState })
)}
>
<span className="inline-flex items-center gap-1.5">
{localState === 'closed' ? (
<CircleDashed className="size-3.5" />
) : (
<CircleDot className="size-3.5" />
)}
{getStateLabel({ ...item, state: localState })}
</span>
<ChevronDown className="size-3 opacity-60" />
</button>
</PopoverTrigger>
<PopoverContent className="w-44 p-1" align="start">
<button
type="button"
onClick={() => handleStateChange('open')}
className={cn(
'flex w-full items-center gap-2 rounded-sm px-2 py-1.5 text-[12px] hover:bg-accent',
localState === 'open' && 'bg-accent/50'
)}
>
<CircleDot className="size-3 text-emerald-500" />
Open
</button>
<button
type="button"
onClick={() => handleStateChange('closed')}
className={cn(
'flex w-full items-center gap-2 rounded-sm px-2 py-1.5 text-[12px] hover:bg-accent',
localState === 'closed' && 'bg-accent/50'
)}
>
<CircleDashed className="size-3 text-rose-500" />
Closed
</button>
</PopoverContent>
</Popover>
</section>
{/* Assignees */}
<section>
<div className="mb-2 flex items-center justify-between text-[11px] font-semibold uppercase tracking-[0.05em] text-muted-foreground">
<span>Assignees</span>
<Popover open={assigneePopoverOpen} onOpenChange={setAssigneePopoverOpen}>
<PopoverTrigger asChild>
<button
type="button"
disabled={isPending('assignees') || repoAssignees.loading}
aria-label="Edit assignees"
className="rounded p-0.5 text-muted-foreground transition hover:bg-accent hover:text-foreground disabled:opacity-50"
>
{isPending('assignees') ? (
<LoaderCircle className="size-3 animate-spin" />
) : (
<Pencil className="size-3" />
)}
</button>
</PopoverTrigger>
<PopoverContent
className="popover-scroll-content scrollbar-sleek w-60 p-1"
align="end"
>
{repoAssignees.error ? (
<div className="px-2 py-3 text-center text-[12px] text-destructive">
{repoAssignees.error}
</div>
) : (
<div>
{repoAssignees.data.map((user) => (
<button
key={user.login}
type="button"
onClick={() => handleAssigneeToggle(user.login)}
className="flex w-full items-center gap-2 rounded-sm px-2 py-1.5 text-[12px] hover:bg-accent"
>
<span
className={cn(
'flex size-3.5 items-center justify-center rounded-sm border',
localAssignees.includes(user.login)
? 'border-primary bg-primary text-primary-foreground'
: 'border-input'
)}
>
{localAssignees.includes(user.login) && checkIcon}
</span>
<span className="min-w-0 flex-1 text-left">
<span className="block truncate">{user.login}</span>
{user.name && (
<span className="block truncate text-[11px] text-muted-foreground">
{user.name}
</span>
)}
</span>
</button>
))}
</div>
)}
</PopoverContent>
</Popover>
</div>
{localAssignees.length === 0 ? (
<div className="text-[12px] text-muted-foreground">No one assigned</div>
) : (
<ul className="flex flex-col gap-1.5">
{localAssignees.map((login) => {
const user = repoAssignees.data.find((u) => u.login === login)
return (
<li key={login} className="flex min-w-0 items-center gap-2">
{user?.avatarUrl ? (
<img
src={user.avatarUrl}
alt=""
className="size-5 shrink-0 rounded-full border border-border/40 object-cover"
/>
) : (
<div className="size-5 shrink-0 rounded-full bg-muted" />
)}
<span className="min-w-0 truncate text-[12px] font-medium text-foreground">
{login}
</span>
</li>
)
})}
</ul>
)}
</section>
{/* Labels */}
<section>
<div className="mb-2 flex items-center justify-between text-[11px] font-semibold uppercase tracking-[0.05em] text-muted-foreground">
<span>Labels</span>
<Popover open={labelPopoverOpen} onOpenChange={setLabelPopoverOpen}>
<PopoverTrigger asChild>
<button
type="button"
disabled={isPending('labels') || repoLabels.loading}
aria-label="Edit labels"
className="rounded p-0.5 text-muted-foreground transition hover:bg-accent hover:text-foreground disabled:opacity-50"
>
{isPending('labels') ? (
<LoaderCircle className="size-3 animate-spin" />
) : (
<Pencil className="size-3" />
)}
</button>
</PopoverTrigger>
<PopoverContent
className="popover-scroll-content scrollbar-sleek w-60 p-1"
align="end"
>
{repoLabels.error ? (
<div className="px-2 py-3 text-center text-[12px] text-destructive">
{repoLabels.error}
</div>
) : (
<div>
{repoLabels.data.map((label) => (
<button
key={label}
type="button"
onClick={() => handleLabelToggle(label)}
className="flex w-full items-center gap-2 rounded-sm px-2 py-1.5 text-[12px] hover:bg-accent"
>
<span
className={cn(
'flex size-3.5 items-center justify-center rounded-sm border',
localLabels.includes(label)
? 'border-primary bg-primary text-primary-foreground'
: 'border-input'
)}
>
{localLabels.includes(label) && checkIcon}
</span>
{label}
</button>
))}
</div>
)}
</PopoverContent>
</Popover>
</div>
{localLabels.length === 0 ? (
<div className="text-[12px] text-muted-foreground">None yet</div>
) : (
<div className="flex flex-wrap gap-1.5">
{localLabels.map((name) => (
<span
key={name}
className="inline-flex items-center rounded-full border border-border/50 bg-muted/40 px-2 py-0.5 text-[11px] font-medium text-foreground"
>
{name}
</span>
))}
</div>
)}
</section>
</aside>
)
}
return (
<div className="flex flex-wrap items-center gap-x-3 gap-y-2 border-b border-border/60 px-4 py-2.5">
{/* State */}
@ -5253,118 +5474,242 @@ export default function GitHubItemDialog({
[details?.pullRequestId, detailsCacheKey, repoPath, workItem]
)
const isIssuePage = variant === 'page' && workItem?.type === 'issue'
const ownerRepo = workItem ? parseOwnerRepoFromItemUrl(workItem.url) : null
const issueStateBadgeTone =
localState === 'closed' ? 'bg-rose-600 text-white' : 'bg-emerald-600 text-white'
const content = workItem ? (
<div className="flex h-full min-h-0 flex-col">
<div className="flex-none border-b border-border/60 bg-card/80 px-4 py-3 shadow-xs backdrop-blur supports-[backdrop-filter]:bg-card/70">
<div className="flex items-start gap-3">
{variant === 'page' ? (
<Button
type="button"
variant="ghost"
size="sm"
onClick={onClose}
className="-ml-1 mt-0.5 shrink-0 gap-1.5"
aria-label={backLabel}
>
<ChevronLeft className="size-4" />
{backLabel}
</Button>
) : null}
<div className="mt-0.5 flex size-8 shrink-0 items-center justify-center rounded-md border border-border/60 bg-muted/40 text-muted-foreground">
<Icon className="size-4" />
</div>
<div className="min-w-0 flex-1 space-y-1">
<div className="flex flex-wrap items-center gap-2 text-[11px] text-muted-foreground">
<WorkItemStateBadge item={{ ...workItem, state: localState }} />
<span className="font-mono">#{workItem.number}</span>
<span>{workItem.type === 'pr' ? 'Pull request' : 'Issue'}</span>
</div>
<h2 className="text-[15px] font-semibold leading-snug text-foreground">
{workItem.title}
</h2>
<div className="flex flex-wrap items-center gap-x-2 gap-y-1 text-[11px] text-muted-foreground">
<span>{workItem.author ?? 'unknown'}</span>
<span>updated {formatRelativeTime(workItem.updatedAt)}</span>
{workItem.branchName && (
<span className="max-w-full truncate rounded-md border border-border/50 bg-muted/40 px-1.5 py-0.5 font-mono text-[10px] text-muted-foreground">
{workItem.branchName}
</span>
)}
</div>
{workItem.type === 'issue' && (
<WorkItemIssueSourceIndicator url={workItem.url} repoId={effectiveRepoId} />
)}
</div>
<div className="flex shrink-0 items-center justify-end gap-1">
{workItem.type === 'pr' && (
{isIssuePage ? (
<>
{/* Row 1: breadcrumb-style strip mirroring GitHub's canvas-subtle header */}
<div className="flex-none border-b border-border/60 bg-muted/30 px-6 py-2.5">
<div className="flex items-center gap-2 text-[13px] text-muted-foreground">
<Button
type="button"
variant="ghost"
size="sm"
onClick={() => onUse(workItem)}
className="gap-1.5 whitespace-nowrap"
aria-label="Start workspace from PR"
onClick={onClose}
className="-ml-2 h-7 gap-1 px-2 text-muted-foreground hover:text-foreground"
aria-label={backLabel}
>
Start workspace from PR
<ArrowRight className="size-3.5" />
<ChevronLeft className="size-4" />
{backLabel}
</Button>
)}
<Tooltip>
<TooltipTrigger asChild>
<span className="text-border">·</span>
{ownerRepo ? (
<>
<span className="truncate">
<span className="text-muted-foreground">{ownerRepo.owner}</span>
<span className="mx-1 text-muted-foreground/60">/</span>
<span className="font-medium text-foreground">{ownerRepo.repo}</span>
</span>
<span className="text-muted-foreground/60">·</span>
</>
) : null}
<span className="font-mono text-muted-foreground">#{workItem.number}</span>
<div className="ml-auto flex items-center gap-1">
<Tooltip>
<TooltipTrigger asChild>
<Button
type="button"
variant="ghost"
size="icon-sm"
onClick={() => void handleCopyWorkItemLink()}
aria-label="Copy GitHub link"
>
{linkCopied ? (
<Check className="size-4 text-emerald-500" />
) : (
<Copy className="size-4" />
)}
</Button>
</TooltipTrigger>
<TooltipContent side="bottom" sideOffset={6}>
{linkCopied ? 'Copied' : 'Copy GitHub link'}
</TooltipContent>
</Tooltip>
<Tooltip>
<TooltipTrigger asChild>
<Button
variant="ghost"
size="icon-sm"
onClick={() => window.api.shell.openUrl(workItem.url)}
aria-label="Open on GitHub"
>
<ExternalLink className="size-4" />
</Button>
</TooltipTrigger>
<TooltipContent side="bottom" sideOffset={6}>
Open on GitHub
</TooltipContent>
</Tooltip>
</div>
</div>
</div>
{/* Row 2: large title block */}
<div className="flex-none border-b border-border/60 bg-card px-6 py-4">
<div className="flex items-start gap-4">
<h1 className="min-w-0 flex-1 text-[28px] font-medium leading-tight text-foreground">
<span className="break-words">{workItem.title}</span>
<span className="ml-2 font-light text-muted-foreground">#{workItem.number}</span>
</h1>
<div className="flex shrink-0 items-center gap-2">
{/* Why: Orca's signature affordance keep this primary so it
stands out against GitHub's familiar surface. */}
<Button
type="button"
variant="ghost"
size="icon-sm"
onClick={() => void handleCopyWorkItemLink()}
aria-label="Copy GitHub link"
size="sm"
onClick={() => onUse(workItem)}
className="gap-1.5 whitespace-nowrap"
aria-label="Start workspace from issue"
>
{linkCopied ? (
<Check className="size-4 text-emerald-500" />
) : (
<Copy className="size-4" />
)}
Start workspace from issue
<ArrowRight className="size-3.5" />
</Button>
</TooltipTrigger>
<TooltipContent side="bottom" sideOffset={6}>
{linkCopied ? 'Copied' : 'Copy GitHub link'}
</TooltipContent>
</Tooltip>
<Tooltip>
<TooltipTrigger asChild>
</div>
</div>
<div className="mt-3 flex flex-wrap items-center gap-2 text-[13px] text-muted-foreground">
<span
className={cn(
'inline-flex items-center gap-1.5 rounded-full px-3 py-1 text-[12px] font-medium',
issueStateBadgeTone
)}
>
{localState === 'closed' ? (
<CircleDashed className="size-3.5" />
) : (
<CircleDot className="size-3.5" />
)}
{localState === 'closed' ? 'Closed' : 'Open'}
</span>
<span className="flex flex-wrap items-center gap-1.5">
<span className="font-semibold text-foreground">
{workItem.author ?? 'unknown'}
</span>
<span>opened this issue</span>
<span className="text-muted-foreground/80">
· updated {formatRelativeTime(workItem.updatedAt)}
</span>
</span>
<WorkItemIssueSourceIndicator url={workItem.url} repoId={effectiveRepoId} />
</div>
</div>
</>
) : (
<div className="flex-none border-b border-border/60 bg-card/80 px-4 py-3 shadow-xs backdrop-blur supports-[backdrop-filter]:bg-card/70">
<div className="flex items-start gap-3">
{variant === 'page' ? (
<Button
type="button"
variant="ghost"
size="sm"
onClick={onClose}
className="-ml-1 mt-0.5 shrink-0 gap-1.5"
aria-label={backLabel}
>
<ChevronLeft className="size-4" />
{backLabel}
</Button>
) : null}
<div className="mt-0.5 flex size-8 shrink-0 items-center justify-center rounded-md border border-border/60 bg-muted/40 text-muted-foreground">
<Icon className="size-4" />
</div>
<div className="min-w-0 flex-1 space-y-1">
<div className="flex flex-wrap items-center gap-2 text-[11px] text-muted-foreground">
<WorkItemStateBadge item={{ ...workItem, state: localState }} />
<span className="font-mono">#{workItem.number}</span>
<span>{workItem.type === 'pr' ? 'Pull request' : 'Issue'}</span>
</div>
<h2 className="text-[15px] font-semibold leading-snug text-foreground">
{workItem.title}
</h2>
<div className="flex flex-wrap items-center gap-x-2 gap-y-1 text-[11px] text-muted-foreground">
<span>{workItem.author ?? 'unknown'}</span>
<span>updated {formatRelativeTime(workItem.updatedAt)}</span>
{workItem.branchName && (
<span className="max-w-full truncate rounded-md border border-border/50 bg-muted/40 px-1.5 py-0.5 font-mono text-[10px] text-muted-foreground">
{workItem.branchName}
</span>
)}
</div>
{workItem.type === 'issue' && (
<WorkItemIssueSourceIndicator url={workItem.url} repoId={effectiveRepoId} />
)}
</div>
<div className="flex shrink-0 items-center justify-end gap-1">
{workItem.type === 'pr' && (
<Button
variant="ghost"
size="icon-sm"
onClick={() => window.api.shell.openUrl(workItem.url)}
aria-label="Open on GitHub"
type="button"
size="sm"
onClick={() => onUse(workItem)}
className="gap-1.5 whitespace-nowrap"
aria-label="Start workspace from PR"
>
<ExternalLink className="size-4" />
Start workspace from PR
<ArrowRight className="size-3.5" />
</Button>
</TooltipTrigger>
<TooltipContent side="bottom" sideOffset={6}>
Open on GitHub
</TooltipContent>
</Tooltip>
{variant === 'sheet' ? (
)}
<Tooltip>
<TooltipTrigger asChild>
<Button
type="button"
variant="ghost"
size="icon-sm"
onClick={() => void handleCopyWorkItemLink()}
aria-label="Copy GitHub link"
>
{linkCopied ? (
<Check className="size-4 text-emerald-500" />
) : (
<Copy className="size-4" />
)}
</Button>
</TooltipTrigger>
<TooltipContent side="bottom" sideOffset={6}>
{linkCopied ? 'Copied' : 'Copy GitHub link'}
</TooltipContent>
</Tooltip>
<Tooltip>
<TooltipTrigger asChild>
<Button
variant="ghost"
size="icon-sm"
onClick={onClose}
aria-label="Close preview"
onClick={() => window.api.shell.openUrl(workItem.url)}
aria-label="Open on GitHub"
>
<X className="size-4" />
<ExternalLink className="size-4" />
</Button>
</TooltipTrigger>
<TooltipContent side="bottom" sideOffset={6}>
Close · Esc
Open on GitHub
</TooltipContent>
</Tooltip>
) : null}
{variant === 'sheet' ? (
<Tooltip>
<TooltipTrigger asChild>
<Button
variant="ghost"
size="icon-sm"
onClick={onClose}
aria-label="Close preview"
>
<X className="size-4" />
</Button>
</TooltipTrigger>
<TooltipContent side="bottom" sideOffset={6}>
Close · Esc
</TooltipContent>
</Tooltip>
) : null}
</div>
</div>
</div>
</div>
)}
{(repoPath || projectOrigin) && (
{!isIssuePage && (repoPath || projectOrigin) && (
<GHEditSection
item={workItem}
repoPath={repoPath}
@ -5397,6 +5742,89 @@ export default function GitHubItemDialog({
<div className="min-h-0 flex-1">
{error ? (
<div className="px-4 py-6 text-[12px] text-destructive">{error}</div>
) : isIssuePage ? (
<div className="h-full min-h-0 overflow-y-auto scrollbar-sleek bg-background">
<div className="mx-auto grid w-full max-w-[1280px] grid-cols-1 gap-8 px-6 py-6 lg:grid-cols-[minmax(0,1fr)_260px]">
<div className="min-w-0">
<ConversationTab
item={displayWorkItem ?? workItem}
repoPath={repoPath}
repoId={effectiveRepoId}
body={body}
comments={comments}
files={files}
headSha={details?.headSha}
baseSha={details?.baseSha}
loading={loading}
detailsLoaded={detailsLoaded}
checks={checks}
participants={details?.participants ?? []}
localState={localState}
onStateChange={setLocalState}
projectOrigin={projectOrigin}
onMutated={() => {
if (repoPath) {
invalidateWorkItemDetailsCacheByMatch({
repoPath,
repoId: effectiveRepoId ?? undefined,
type: workItem.type,
number: workItem.number
})
}
}}
onChecksUpdated={(nextChecks) => {
if (detailsCacheKey) {
patchCachedPRChecks(detailsCacheKey, nextChecks)
}
}}
onBodyUpdated={(nextBody) => {
if (detailsCacheKey) {
patchCachedWorkItemBody(detailsCacheKey, nextBody)
}
}}
onCommentAdded={appendOptimisticComment}
onReviewersRequested={(nextReviewRequests) => {
if (detailsCacheKey) {
patchCachedPRReviewRequests(detailsCacheKey, nextReviewRequests)
}
onReviewRequestsChange?.(
{ id: workItem.id, repoId: workItem.repoId },
nextReviewRequests
)
}}
/>
</div>
{(repoPath || projectOrigin) && (
<div className="min-w-0">
<div className="lg:sticky lg:top-4">
<GHEditSection
item={workItem}
repoPath={repoPath}
repoId={effectiveRepoId}
projectOrigin={projectOrigin}
localState={localState}
localLabels={localLabels}
onStateChange={setLocalState}
onLabelsChange={setLocalLabels}
onMutated={() => {
if (repoPath) {
invalidateWorkItemDetailsCacheByMatch({
repoPath,
repoId: effectiveRepoId ?? undefined,
type: workItem.type,
number: workItem.number
})
}
}}
assignees={details?.assignees ?? []}
onUse={onUse}
layout="sidebar"
/>
</div>
</div>
)}
</div>
</div>
) : (
<Tabs
value={tab}

File diff suppressed because it is too large Load Diff

View File

@ -92,6 +92,7 @@ import PRFilterDropdowns, { type PRFilterChange } from '@/components/github/PRFi
import { parseGitHubIssueOrPRLink } from '@/lib/github-links'
import { useRepoAssigneesBySlug } from '@/hooks/useGitHubSlugMetadata'
import GitHubItemDialog, { type ItemDialogTab } from '@/components/GitHubItemDialog'
import PullRequestPage from '@/components/PullRequestPage'
import GitLabItemDialog from '@/components/GitLabItemDialog'
import ProjectViewWrapper from '@/components/github-project/ProjectViewWrapper'
import LinearIssueWorkspace from '@/components/LinearIssueWorkspace'
@ -4132,7 +4133,15 @@ export default function TaskPage(): React.JSX.Element {
cluster (6 + 16 = 22). The previous pt-3 placed the cluster 6px
too low, breaking the visual band across the top chrome. */}
<div className="mx-auto flex min-h-0 min-w-0 w-full flex-1 flex-col px-5 pt-1.5 pb-5 md:px-8 md:pt-1.5 md:pb-7">
<div className="flex-none flex flex-col gap-3">
<div
className={cn(
'flex-none flex flex-col gap-3',
// Why: GitHub detail views (PR + Issue) fill the entire right
// pane — the list filter row above them is not relevant and
// would visually duplicate the detail page's own breadcrumb.
taskSource === 'github' && dialogWorkItem && 'hidden'
)}
>
<section className="flex flex-col gap-3">
<div className="flex flex-col gap-3">
<div className="flex items-center justify-between gap-2">
@ -4740,20 +4749,36 @@ export default function TaskPage(): React.JSX.Element {
</div>
{taskSource === 'github' && dialogWorkItem ? (
<GitHubItemDialog
workItem={dialogWorkItem}
initialTab={dialogInitialTab}
repoPath={dialogRepoPath}
repoId={dialogWorkItem.repoId}
variant="page"
backLabel="GitHub list"
onUse={(item) => {
setDialogWorkItem(null)
handleUseWorkItem(item)
}}
onReviewRequestsChange={handleDialogReviewRequestsChange}
onClose={closeTaskDetailPage}
/>
dialogWorkItem.type === 'pr' ? (
<PullRequestPage
workItem={dialogWorkItem}
initialTab={dialogInitialTab}
repoPath={dialogRepoPath}
repoId={dialogWorkItem.repoId}
backLabel="Pull requests"
onUse={(item) => {
setDialogWorkItem(null)
handleUseWorkItem(item)
}}
onReviewRequestsChange={handleDialogReviewRequestsChange}
onClose={closeTaskDetailPage}
/>
) : (
<GitHubItemDialog
workItem={dialogWorkItem}
initialTab={dialogInitialTab}
repoPath={dialogRepoPath}
repoId={dialogWorkItem.repoId}
variant="page"
backLabel="GitHub list"
onUse={(item) => {
setDialogWorkItem(null)
handleUseWorkItem(item)
}}
onReviewRequestsChange={handleDialogReviewRequestsChange}
onClose={closeTaskDetailPage}
/>
)
) : taskSource === 'github' && githubMode === 'project' ? (
<div className="mt-3 flex min-h-0 min-w-0 max-h-full flex-col overflow-hidden rounded-md border border-border/50 bg-muted/50 shadow-sm">
<ProjectViewWrapper />