From e2c8a66ee0e1e9dd2c51265163eadcf659e5319a Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Sat, 20 Jun 2026 01:15:58 -0700 Subject: [PATCH] fix: address review findings (#5889) --- .../sidebar/WorktreeCard.pr-display.test.tsx | 5 ++- .../src/components/sidebar/WorktreeCard.tsx | 7 ++-- .../sidebar/WorktreeCardStatusSlot.test.tsx | 30 +++++++------- .../sidebar/WorktreeCardStatusSlot.tsx | 10 ++--- .../sidebar/WorktreeContextMenu.test.ts | 11 ++++++ .../sidebar/WorktreeContextMenu.tsx | 39 ++++++++++++------- 6 files changed, 64 insertions(+), 38 deletions(-) diff --git a/src/renderer/src/components/sidebar/WorktreeCard.pr-display.test.tsx b/src/renderer/src/components/sidebar/WorktreeCard.pr-display.test.tsx index 27ad01204..ee145d69d 100644 --- a/src/renderer/src/components/sidebar/WorktreeCard.pr-display.test.tsx +++ b/src/renderer/src/components/sidebar/WorktreeCard.pr-display.test.tsx @@ -203,8 +203,9 @@ describe('WorktreeCard linked PR display', () => { ) - expect(unreadMarkup).toContain('aria-label="Mark as read"') - expect(unreadMarkup).toContain('PR checks: Failed · Mark read') + expect(unreadMarkup).not.toContain('aria-label="Mark as read"') + expect(unreadMarkup).toContain('PR checks: Failed') + expect(unreadMarkup).not.toContain('Mark read') expect(unreadMarkup).toContain('size-[13px] translate-x-px') expect(unreadMarkup).not.toContain('lucide-bell') expect(unreadMarkup).not.toContain('text-amber-500') diff --git a/src/renderer/src/components/sidebar/WorktreeCard.tsx b/src/renderer/src/components/sidebar/WorktreeCard.tsx index 5903804cd..beaadc987 100644 --- a/src/renderer/src/components/sidebar/WorktreeCard.tsx +++ b/src/renderer/src/components/sidebar/WorktreeCard.tsx @@ -1050,9 +1050,9 @@ const WorktreeCard = React.memo(function WorktreeCard({ const showConflictOperationBadge = !!conflictOperation && conflictOperation !== 'unknown' && conflictOperation !== 'rebase' const hasMetadataBadge = showConflictOperationBadge || hasAutomationMetadata - const showUnreadQuickAction = !affiliateListMode && showStatus - // Why: the slot owns the tiny unread/status lane; legacy keeps the bell, - // while the experimental card keeps the status glyph visible. + const showUnreadQuickAction = !affiliateListMode && showStatus && !newCardStyle + // Why: the slot owns the tiny unread/status lane; legacy keeps the bell + // toggle, while the experimental card keeps the status glyph passive. const showCombinedStatusSlot = showStatus const showTitleRowPrimary = compactCards && worktree.isMainWorktree && !isFolder const showMetaRowDetails = !newCardStyle && !compactCards && (hasDetails || hasPorts) @@ -1763,6 +1763,7 @@ const WorktreeCard = React.memo(function WorktreeCard({ ) : ( diff --git a/src/renderer/src/components/sidebar/WorktreeCardStatusSlot.test.tsx b/src/renderer/src/components/sidebar/WorktreeCardStatusSlot.test.tsx index be513d6f3..595d5d248 100644 --- a/src/renderer/src/components/sidebar/WorktreeCardStatusSlot.test.tsx +++ b/src/renderer/src/components/sidebar/WorktreeCardStatusSlot.test.tsx @@ -58,7 +58,7 @@ describe('WorktreeCardStatusSlot', () => { expect(markup).toContain('text-amber-500') }) - it('keeps the status dot visible for unread rows when new card style is on', () => { + it('keeps the status dot passive for unread rows when new card style is on', () => { const markup = renderToStaticMarkup( { /> ) - expect(markup).toContain('aria-label="Mark as read"') - expect(markup).toContain('Active · Mark as read') + expect(markup).not.toContain('aria-label="Mark as read"') + expect(markup).not.toContain('Mark as read') + expect(markup).toContain('Active') expect(markup).toContain('bg-emerald-500') expect(markup).not.toContain('lucide-bell') expect(markup).not.toContain('text-amber-500') @@ -307,7 +308,7 @@ describe('WorktreeCardStatusSlot', () => { expect(markup).toContain('text-amber-500') }) - it('keeps PR status visible for unread rows when new card style is on', () => { + it('keeps PR status passive for unread rows when new card style is on', () => { const markup = renderToStaticMarkup( { /> ) - expect(markup).toContain('aria-label="Mark as read"') - expect(markup).toContain('Mark as read') - expect(markup).toContain('PR checks: Failed · Mark as read') - expect(markup).toContain( - 'group/unread relative flex cursor-pointer items-center justify-center rounded transition-all size-5' - ) + expect(markup).not.toContain('aria-label="Mark as read"') + expect(markup).not.toContain('Mark as read') + expect(markup).toContain('PR checks: Failed') + expect(markup).not.toContain('group/unread') + expect(markup).not.toContain('cursor-pointer') expect(markup).toContain('text-rose-500/85') expect(markup).not.toContain('lucide-bell') expect(markup).not.toContain('text-amber-500') expect(markup).not.toContain('bg-emerald-500') }) - it('keeps the branch icon visible for unread rows in new card style', () => { + it('keeps the branch icon passive for unread rows in new card style', () => { const markup = renderToStaticMarkup( { /> ) - expect(markup).toContain('Branch · Mark as read') - expect(markup).toContain( - 'group/unread relative flex cursor-pointer items-center justify-center rounded transition-all size-5' - ) + expect(markup).toContain('Branch') + expect(markup).not.toContain('Mark as read') + expect(markup).not.toContain('group/unread') + expect(markup).not.toContain('cursor-pointer') expect(markup).toContain('lucide-git-branch') expect(markup).not.toContain('lucide-bell') expect(markup).not.toContain('text-amber-500') diff --git a/src/renderer/src/components/sidebar/WorktreeCardStatusSlot.tsx b/src/renderer/src/components/sidebar/WorktreeCardStatusSlot.tsx index c7ab75ecf..38695d400 100644 --- a/src/renderer/src/components/sidebar/WorktreeCardStatusSlot.tsx +++ b/src/renderer/src/components/sidebar/WorktreeCardStatusSlot.tsx @@ -132,19 +132,19 @@ export function WorktreeCardStatusSlot({ ) - if (!showStatus && !showUnreadAction) { + const unreadActionEnabled = showUnreadAction && !newCardStyle + + if (!showStatus && !unreadActionEnabled) { return null } - if (!showUnreadAction) { + if (!unreadActionEnabled) { return passiveStatus } const actionLabel = isUnread ? 'Mark as read' : 'Mark as unread' const tooltip = - showStatus && (!isUnread || newCardStyle) - ? `${passiveStatusLabel} · ${unreadTooltip}` - : unreadTooltip + showStatus && !isUnread ? `${passiveStatusLabel} · ${unreadTooltip}` : unreadTooltip return ( <> diff --git a/src/renderer/src/components/sidebar/WorktreeContextMenu.test.ts b/src/renderer/src/components/sidebar/WorktreeContextMenu.test.ts index 47a67ce29..12599505c 100644 --- a/src/renderer/src/components/sidebar/WorktreeContextMenu.test.ts +++ b/src/renderer/src/components/sidebar/WorktreeContextMenu.test.ts @@ -7,6 +7,7 @@ import { shouldRemoveProjectFromContextMenu, shouldSuppressContextMenuFollowUpClick, shouldContinueDeleteSiblingPositionRestore, + shouldShowReadToggleContextMenuItem, getWorktreeParentPickerAnchor, getWorktreeParentPickerLabel, isWorktreeParentPickerDisabled @@ -98,6 +99,16 @@ describe('shouldSuppressContextMenuFollowUpClick', () => { }) }) +describe('shouldShowReadToggleContextMenuItem', () => { + it('keeps the read toggle in legacy card menus', () => { + expect(shouldShowReadToggleContextMenuItem({ newCardStyle: false })).toBe(true) + }) + + it('hides the read toggle in experimental card menus', () => { + expect(shouldShowReadToggleContextMenuItem({ newCardStyle: true })).toBe(false) + }) +}) + describe('shouldContinueDeleteSiblingPositionRestore', () => { it('stops once the delete row position has settled even when the row remains mounted', () => { expect( diff --git a/src/renderer/src/components/sidebar/WorktreeContextMenu.tsx b/src/renderer/src/components/sidebar/WorktreeContextMenu.tsx index a153778f0..fa29c8bbd 100644 --- a/src/renderer/src/components/sidebar/WorktreeContextMenu.tsx +++ b/src/renderer/src/components/sidebar/WorktreeContextMenu.tsx @@ -57,6 +57,7 @@ type Props = { worktree: Worktree children: React.ReactNode contentClassName?: string + newCardStyle?: boolean selectedWorktrees?: readonly Worktree[] onContextMenuSelect?: (event: React.MouseEvent) => readonly Worktree[] onOpenChange?: (open: boolean) => void @@ -123,6 +124,10 @@ function isWorktreeParentPickerDisabled(args: { return args.isDeleting || args.eligibleParentCount === 0 } +function shouldShowReadToggleContextMenuItem(args: { newCardStyle: boolean }): boolean { + return !args.newCardStyle +} + function getWorktreeParentPickerAnchor( scope: HTMLElement | null, worktreeId: string @@ -243,6 +248,7 @@ const WorktreeContextMenu = React.memo(function WorktreeContextMenu({ worktree, children, contentClassName, + newCardStyle = false, selectedWorktrees, onContextMenuSelect, onOpenChange @@ -344,6 +350,7 @@ const WorktreeContextMenu = React.memo(function WorktreeContextMenu({ (item) => worktreeLineageById[item.id] || workspaceLineageByChildKey[worktreeWorkspaceKey(item.id)] ) + const showReadToggle = shouldShowReadToggleContextMenuItem({ newCardStyle }) const eligibleParentCount = useMemo( () => getEligibleWorktreeParents({ @@ -664,19 +671,24 @@ const WorktreeContextMenu = React.memo(function WorktreeContextMenu({ ? translate('auto.components.sidebar.WorktreeContextMenu.697d0f6e1b', 'Unpin') : translate('auto.components.sidebar.WorktreeContextMenu.3baa7d6507', 'Pin')} - - {worktree.isUnread ? ( - - ) : ( - - )} - {worktree.isUnread - ? translate('auto.components.sidebar.WorktreeContextMenu.8dacff1fe0', 'Mark Read') - : translate( - 'auto.components.sidebar.WorktreeContextMenu.f50603c6b2', - 'Mark Unread' - )} - + {showReadToggle && ( + + {worktree.isUnread ? ( + + ) : ( + + )} + {worktree.isUnread + ? translate( + 'auto.components.sidebar.WorktreeContextMenu.8dacff1fe0', + 'Mark Read' + ) + : translate( + 'auto.components.sidebar.WorktreeContextMenu.f50603c6b2', + 'Mark Unread' + )} + + )} {repo ? ( <> @@ -907,6 +919,7 @@ export { getWorktreeParentPickerLabel, isWorktreeParentPickerDisabled, shouldRemoveProjectFromContextMenu, + shouldShowReadToggleContextMenuItem, shouldUseNativeContextMenu, shouldSuppressContextMenuFollowUpClick, shouldIgnoreNestedWorktreeContextMenuScope