fix: address review findings (#455)

This commit is contained in:
Jinjing 2026-04-10 19:37:36 -07:00 committed by GitHub
parent 02ac8f48e9
commit aef508d2b3
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
4 changed files with 62 additions and 91 deletions

View File

@ -100,33 +100,36 @@ const components: Components = {
// with existing plain-text comments that rely on newline formatting.
const remarkPlugins = [remarkGfm, remarkBreaks]
type CommentMarkdownProps = {
type CommentMarkdownProps = React.ComponentPropsWithoutRef<'div'> & {
content: string
className?: string
onClick?: (e: React.MouseEvent) => void
}
const CommentMarkdown = React.memo(function CommentMarkdown({
content,
className,
onClick
}: CommentMarkdownProps) {
return (
<div
className={cn(
// Reset inline-code pill styles when <code> is inside a <pre> block.
// The descendant selector (pre code) has higher specificity than the
// direct utility classes on <code>, so these overrides win reliably.
'[&_pre_code]:bg-transparent [&_pre_code]:p-0 [&_pre_code]:rounded-none',
className
)}
onClick={onClick}
>
<Markdown remarkPlugins={remarkPlugins} components={components}>
{content}
</Markdown>
</div>
)
})
// Why forwardRef + rest props: Radix's HoverCardTrigger asChild merges a ref
// and event handlers (onPointerEnter, onPointerLeave, data-state, etc.) onto
// the child. Without forwarding both, the hover card cannot open or position.
const CommentMarkdown = React.memo(
React.forwardRef<HTMLDivElement, CommentMarkdownProps>(function CommentMarkdown(
{ content, className, ...rest },
ref
) {
return (
<div
ref={ref}
className={cn(
// Reset inline-code pill styles when <code> is inside a <pre> block.
// The descendant selector (pre code) has higher specificity than the
// direct utility classes on <code>, so these overrides win reliably.
'[&_pre_code]:bg-transparent [&_pre_code]:p-0 [&_pre_code]:rounded-none',
className
)}
{...rest}
>
<Markdown remarkPlugins={remarkPlugins} components={components}>
{content}
</Markdown>
</div>
)
})
)
export default CommentMarkdown

View File

@ -559,11 +559,25 @@ const WorktreeCard = React.memo(function WorktreeCard({
)}
{cardProps.includes('comment') && worktree.comment && (
<CommentMarkdown
content={worktree.comment}
className="text-[11px] text-muted-foreground break-words cursor-pointer -mx-1.5 px-1.5 py-0.5 hover:bg-background/40 hover:text-foreground rounded transition-colors leading-normal [&_.comment-md-p]:block [&_.comment-md-p+.comment-md-p]:mt-1"
onClick={handleEditComment}
/>
<HoverCard openDelay={400}>
<HoverCardTrigger asChild>
<CommentMarkdown
content={worktree.comment}
className="text-[11px] text-muted-foreground break-words -mx-1.5 px-1.5 py-0.5 rounded transition-colors leading-normal line-clamp-2 [&_.comment-md-p]:inline [&_.comment-md-p+.comment-md-p]:before:content-['_']"
onDoubleClick={handleEditComment}
/>
</HoverCardTrigger>
<HoverCardContent
side="right"
align="start"
className="w-72 max-h-80 overflow-y-auto p-3"
>
<CommentMarkdown
content={worktree.comment}
className="text-[11.5px] text-foreground break-words leading-normal [&_.comment-md-p]:block [&_.comment-md-p+.comment-md-p]:mt-1"
/>
</HoverCardContent>
</HoverCard>
)}
</div>
)}

View File

@ -84,27 +84,21 @@ describe('estimateRowHeight', () => {
expect(estimateRowHeight(itemRow(worktree), ['pr'], repoMap, prCache)).toBe(55)
})
it('estimates comment height based on content length', () => {
it('estimates comment height as fixed 2-line clamp', () => {
const wt = { ...worktree, comment: 'todo: fix bug' }
const base = estimateRowHeight(itemRow(worktree), ['comment'], repoMap, null)
const withComment = estimateRowHeight(itemRow(wt), ['comment'], repoMap, null)
// 1 line × ceil(16.5)=17 + 4px py-0.5 + 8px meta spacing = 29
expect(withComment - base).toBe(29)
// 2 lines × ceil(16.5)=33 + 4px py-0.5 + 8px meta spacing = 45
expect(withComment - base).toBe(45)
})
it('estimates multi-line comment height from newlines', () => {
const wt = { ...worktree, comment: 'first line\nsecond line\nthird line' }
const h = estimateRowHeight(itemRow(wt), ['comment'], repoMap, null)
// base 55 + ceil(3 × 16.5)=50 + 4px py-0.5 + 8px meta spacing = 117
expect(h).toBe(117)
})
it('estimates wrapped long lines in comment', () => {
// 70 chars wraps to 2 lines at ~35 chars/line
const wt = { ...worktree, comment: 'a'.repeat(70) }
const h = estimateRowHeight(itemRow(wt), ['comment'], repoMap, null)
// base 55 + ceil(2 × 16.5)=33 + 4px py-0.5 + 8px meta spacing = 100
expect(h).toBe(100)
it('uses same fixed height for long comments (clamped to 2 lines)', () => {
const short = { ...worktree, comment: 'short' }
const long = { ...worktree, comment: 'first line\nsecond line\nthird line\nfourth line' }
const hShort = estimateRowHeight(itemRow(short), ['comment'], repoMap, null)
const hLong = estimateRowHeight(itemRow(long), ['comment'], repoMap, null)
// Both clamped to 2 lines, so same height
expect(hShort).toBe(hLong)
})
it('stacks all metadata lines correctly', () => {
@ -113,9 +107,8 @@ describe('estimateRowHeight', () => {
'/tmp/orca::feature/cool': { data: { number: 1 } }
}
const h = estimateRowHeight(itemRow(wt), ['issue', 'pr', 'comment'], repoMap, prCache)
// 55 base + 16 issue + 16 pr + (17+4) comment + 8 meta spacing + 2×3 gaps = 122
// (inter-card gap is handled by the virtualizer's `gap` option, not here)
expect(h).toBe(122)
// 55 base + 16 issue + 16 pr + (33+4) comment + 8 meta spacing + 2×3 gaps = 138
expect(h).toBe(138)
})
it('strips refs/heads/ prefix when building PR cache key', () => {

View File

@ -48,48 +48,9 @@ export function estimateRowHeight(
}
}
if (cardProps.includes('comment') && wt.comment) {
// Comment renders as markdown via react-markdown. Markdown block elements
// (lists, code blocks, blockquotes) add some vertical overhead compared to
// raw text, but the dominant factor is still line count. We estimate visual
// lines from explicit newlines and character wrapping (~35 chars per line at
// typical sidebar width), then add a small buffer for markdown block spacing.
// Line-height is leading-normal (1.5 × 11px = 16.5px) + py-0.5(4px).
const lines = wt.comment.split('\n')
let totalLines = 0
let hasBlocks = false
let inCodeFence = false
let codeFenceLines = 0
for (const line of lines) {
if (line.startsWith('```')) {
hasBlocks = true
if (inCodeFence) {
// Closing fence: cap at max-h-32 (128px) ÷ 16.5 ≈ 8 visible lines
totalLines += Math.min(codeFenceLines, 8)
codeFenceLines = 0
}
inCodeFence = !inCodeFence
continue
}
if (inCodeFence) {
codeFenceLines++
} else {
totalLines += Math.max(1, Math.ceil(line.length / 35))
// Detect markdown block elements that add extra vertical spacing.
// Only check outside code fences — fenced content renders as plain code.
if (/^(\s*[-*+]\s|#{1,6}\s|>\s|---|\d+\.\s)/.test(line)) {
hasBlocks = true
}
}
}
// Handle unclosed code fence (treat remaining lines normally)
if (inCodeFence) {
totalLines += Math.min(codeFenceLines, 8)
}
h += Math.ceil(totalLines * 16.5) + 4
// Markdown blocks (lists, headings, code fences) add ~4-8px of extra margin
if (hasBlocks) {
h += 8
}
// Comment is clamped to 2 lines (line-clamp-2) in the card. Full content
// is shown in a hover card. Height: 2 lines × 16.5px + py-0.5(4px).
h += Math.ceil(2 * 16.5) + 4
metaCount++
}