Skip to content

feat(ui): change session action menu UI style#33

Merged
tiann merged 3 commits intotiann:mainfrom
waynexia:menu-pop-up
Jan 8, 2026
Merged

feat(ui): change session action menu UI style#33
tiann merged 3 commits intotiann:mainfrom
waynexia:menu-pop-up

Conversation

@waynexia
Copy link
Copy Markdown
Contributor

@waynexia waynexia commented Jan 4, 2026

A hover pop-up looks a bit strange to me. This change makes it look more like a menu from overflow dots.

image

If this looks good, I can further change the menu of the session list page to keep them consistent.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors the session action menu from a centered modal dialog to a positioned dropdown menu that appears relative to a trigger button. The change provides a more contextual, menu-like experience that aligns with typical overflow menu patterns.

  • Replaces Dialog component with custom positioned menu using fixed positioning
  • Adds animation and smart positioning logic that adjusts for viewport constraints
  • Updates trigger buttons in SessionHeader and SessionList to toggle menu and provide anchor reference

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.

File Description
web/src/index.css Adds menu-pop keyframe animation and utility class for menu appearance animation
web/src/components/SessionActionMenu.tsx Completely refactors from Dialog-based modal to custom positioned dropdown with viewport-aware positioning, click-outside handling, and keyboard support
web/src/components/SessionHeader.tsx Updates menu trigger button to toggle (instead of open-only), adds ref for positioning anchor, and includes ARIA attributes
web/src/components/SessionList.tsx Adds ref to session list item button for menu positioning anchor and passes alignment props

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +201 to +207
const menuStyle: CSSProperties | undefined = menuPosition
? {
top: menuPosition.top,
left: menuPosition.left,
transformOrigin: menuPosition.transformOrigin
}
: undefined
Copy link

Copilot AI Jan 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The menu initially renders without position styles when menuPosition is null, which could cause a brief flash or incorrect positioning during the animation. Consider setting an initial position synchronously or hiding the menu until positioned. The animation starts immediately via the animate-menu-pop class, but the position might not be calculated until after the first frame.

Copilot uses AI. Check for mistakes.
Comment on lines +152 to +155
top = Math.min(Math.max(top, padding), viewportHeight - menuRect.height - padding)
left = Math.min(Math.max(left, padding), viewportWidth - menuRect.width - padding)

setMenuPosition({ top, left, transformOrigin })
Copy link

Copilot AI Jan 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the menu position is constrained by viewport bounds (lines 152-153), the calculated transformOrigin (set at lines 133, 145, 148) may no longer accurately reflect the menu's position relative to the anchor element. For example, if a menu is positioned to open below and align to the right (transformOrigin: "top right") but is then pushed left due to viewport constraints, the transform origin should be adjusted accordingly to maintain the correct animation origin point.

Suggested change
top = Math.min(Math.max(top, padding), viewportHeight - menuRect.height - padding)
left = Math.min(Math.max(left, padding), viewportWidth - menuRect.width - padding)
setMenuPosition({ top, left, transformOrigin })
const originalTop = top
const originalLeft = left
const clampedTop = Math.min(
Math.max(top, padding),
viewportHeight - menuRect.height - padding
)
const clampedLeft = Math.min(
Math.max(left, padding),
viewportWidth - menuRect.width - padding
)
let [verticalOrigin, horizontalOrigin] = transformOrigin.split(' ') as [
'top' | 'bottom' | 'center',
'left' | 'right' | 'center'
]
if (clampedTop !== originalTop) {
if (clampedTop <= padding) {
verticalOrigin = 'top'
} else if (clampedTop >= viewportHeight - menuRect.height - padding) {
verticalOrigin = 'bottom'
}
}
if (clampedLeft !== originalLeft) {
if (clampedLeft <= padding) {
horizontalOrigin = 'left'
} else if (clampedLeft >= viewportWidth - menuRect.width - padding) {
horizontalOrigin = 'right'
}
}
const adjustedTransformOrigin = `${verticalOrigin} ${horizontalOrigin}`
setMenuPosition({
top: clampedTop,
left: clampedLeft,
transformOrigin: adjustedTransformOrigin
})

Copilot uses AI. Check for mistakes.
Comment thread web/src/components/SessionActionMenu.tsx
Comment thread web/src/components/SessionActionMenu.tsx Outdated
Comment on lines +189 to +195
window.addEventListener('scroll', handleReflow, true)

return () => {
document.removeEventListener('pointerdown', handlePointerDown)
document.removeEventListener('keydown', handleKeyDown)
window.removeEventListener('resize', handleReflow)
window.removeEventListener('scroll', handleReflow, true)
Copy link

Copilot AI Jan 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The scroll event listener is added with capture phase (true) which will trigger on any scroll in any scrollable container. This could cause unnecessary position recalculations when scrolling unrelated parts of the page. Consider checking if the scroll event affects the anchor or menu visibility, or use a more targeted scroll container reference if available.

Suggested change
window.addEventListener('scroll', handleReflow, true)
return () => {
document.removeEventListener('pointerdown', handlePointerDown)
document.removeEventListener('keydown', handleKeyDown)
window.removeEventListener('resize', handleReflow)
window.removeEventListener('scroll', handleReflow, true)
window.addEventListener('scroll', handleReflow)
return () => {
document.removeEventListener('pointerdown', handlePointerDown)
document.removeEventListener('keydown', handleKeyDown)
window.removeEventListener('resize', handleReflow)
window.removeEventListener('scroll', handleReflow)

Copilot uses AI. Check for mistakes.
<div
ref={menuRef}
role="menu"
aria-label="Session actions"
Copy link

Copilot AI Jan 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The menu container has role="menu" but is missing tabIndex attribute. To be keyboard accessible and focusable, the container should have tabIndex={-1} so it can receive focus programmatically without being in the natural tab order. This works together with focus management to allow keyboard navigation within the menu.

Suggested change
aria-label="Session actions"
aria-label="Session actions"
tabIndex={-1}

Copilot uses AI. Check for mistakes.
Comment thread web/src/components/SessionHeader.tsx
@waynexia
Copy link
Copy Markdown
Contributor Author

waynexia commented Jan 5, 2026

I resolved some, others seem unnecessary/false-positive

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
@tiann tiann merged commit c044676 into tiann:main Jan 8, 2026
1 check passed
@waynexia waynexia deleted the menu-pop-up branch January 8, 2026 03:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants