🎚️ feat: Add Focus Management and Drag-to-Scrub to MessageNav#13497
Conversation
Resolves #13491: move keyboard focus into the conversation when a message indicator is selected, and add a Shift+Alt+M shortcut to jump focus back to the nav. Also adds drag-to-scrub interaction across the rib column.
There was a problem hiding this comment.
Pull request overview
This PR enhances the MessageNav (message “rib” indicator) to improve accessibility and usability by moving focus into the conversation when a rib is selected and adding drag-to-scrub scrolling behavior.
Changes:
- Move keyboard/mouse focus to the target message when selecting a
MessageNavindicator, and add aShift+Alt+Mshortcut to return focus to the current indicator. - Add drag-to-scrub behavior on the rib column with a movement threshold, click-suppression after drag, and shared scroll target calculation.
- Standardize message focus ring styling to use
focus-visibleinstead offocus.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| client/src/components/Messages/ContentRender.tsx | Switch message focus ring styles from focus to focus-visible. |
| client/src/components/Chat/Messages/ui/MessageRender.tsx | Switch message focus ring styles from focus to focus-visible. |
| client/src/components/Chat/Messages/MessageParts.tsx | Add focus-visible ring classes to message wrapper for consistent focus styling. |
| client/src/components/Chat/Messages/MessageNav.tsx | Add focus management, keyboard shortcut, drag-to-scrub interaction, and refactor scroll targeting. |
| client/src/components/Chat/Messages/tests/MessageNav.spec.tsx | Extend MessageNav tests for focus behavior, shortcut handling, and drag scrubbing with a PointerEvent polyfill. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const handlePointerDown = useCallback((e: React.PointerEvent<HTMLDivElement>) => { | ||
| if (e.button !== 0) { | ||
| return; | ||
| } | ||
| suppressClickRef.current = false; | ||
| dragStateRef.current = { pointerId: e.pointerId, startY: e.clientY, dragging: false }; | ||
| }, []); |
GitNexus: 🚀 deployedThe |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7bf8cd5fff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return; | ||
| } | ||
| draggingRef.current = false; | ||
| suppressClickRef.current = true; |
There was a problem hiding this comment.
After a scrub drag ends, this sets suppressClickRef and the flag is only cleared inside handleSelect. If the drag finishes over the column gap/outside an indicator, or the browser suppresses the synthetic click after the movement, no indicator click consumes the flag, so the user's next intentional indicator click is ignored. This makes drag-to-scroll intermittently leave the nav in a one-click-dead state; clear the flag after the expected click window or only suppress the actual post-drag click event.
Useful? React with 👍 / 👎.
|
|
||
| useEffect(() => { | ||
| const onKeyDown = (e: KeyboardEvent) => { | ||
| if (e.altKey && e.shiftKey && e.code === 'KeyM') { |
There was a problem hiding this comment.
Because the nav advertises aria-keyshortcuts="Shift+Alt+M", users on non-QWERTY or alternative keyboard layouts will try the key that produces M, but this handler checks KeyboardEvent.code, which is the physical key position and ignores the user's layout. In those layouts the announced shortcut can fail while a different physical key unexpectedly works, so use a layout-aware check such as e.key for the printable key you expose to assistive technology.
Useful? React with 👍 / 👎.
| } | ||
| state.dragging = true; | ||
| draggingRef.current = true; | ||
| columnRef.current?.setPointerCapture?.(e.pointerId); |
There was a problem hiding this comment.
When a mouse press starts on the rib column, moves out before crossing the 4px threshold, and is released outside, this element never captured the pointer and therefore never receives pointerup, leaving dragStateRef stale. A later hover pointermove over the column can reuse the same mouse pointerId, cross the old threshold, and call setPointerCapture for a pointer that is no longer active, which throws a NotFoundError and breaks the nav handler; capture on pointerdown or clear stale state when e.buttons no longer includes the primary button.
Useful? React with 👍 / 👎.
…icks Addresses Codex/Copilot review on #13497: - Ignore pre-drag pointermoves when the primary button is not held, preventing a stale press (released outside the column) from starting a spurious scrub or calling setPointerCapture on an inactive pointer. - Clear the post-drag click-suppression flag after the synthetic-click window so a later activation (including keyboard) is never swallowed. - Match the advertised Shift+Alt+M shortcut by accepting the layout-aware key in addition to the physical code.
|
@codex review |
danny-avila
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb9a8d8424
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| state.dragging = true; | ||
| draggingRef.current = true; | ||
| columnRef.current?.setPointerCapture?.(e.pointerId); |
There was a problem hiding this comment.
When a drag starts on the very narrow rib column and the pointer leaves the column before the 4px threshold is crossed, this handler has not captured the pointer yet, so subsequent pointermove/pointerup events are delivered to whatever is under the pointer instead of the column. In that diagonal or touch drag scenario, scrubbing never starts and the drag state can remain pending until a later hover move clears it, so drag-to-scroll only works reliably if the pointer stays over the ribs. Capture on pointer down, then release it if the interaction ends without passing the threshold.
Useful? React with 👍 / 👎.
GitNexus: 🚀 deployedThe |
Round-2 Codex review on #13497: the 4px threshold was applied before any pointer capture, so a drag that left the narrow rib column before crossing it silently failed to scrub. Replace per-element capture with document-level pointermove/up/cancel listeners attached on pointerdown: - Drag tracking continues regardless of pointer position (fixes diagonal/touch drags off the ribs). - pointerup is always received, so no stale drag state and no setPointerCapture on an inactive pointer (removes the NotFoundError path entirely). - Native click is preserved, so the keyboard/click selection a11y path is unchanged. - Listeners are torn down on pointerup/cancel and on unmount.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c366beb580
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| finish(state.dragging); | ||
| } | ||
|
|
||
| dragCleanupRef.current = () => finish(false); |
There was a problem hiding this comment.
When a second pointerdown (for example, another finger on a touch laptop/tablet) arrives after the first pointer has crossed the drag threshold, handlePointerDown invokes this cleanup before starting the new drag state. Because the cleanup always calls finish(false), the previous active drag never clears draggingRef.current, so the scroll effect's if (draggingRef.current) return path keeps the rib column from auto-centering until a later successful drag ends. Capture state.dragging here or explicitly clear draggingRef when replacing an active drag.
Useful? React with 👍 / 👎.
| if (e.altKey && e.shiftKey && (e.code === 'KeyM' || e.key.toLowerCase() === 'm')) { | ||
| e.preventDefault(); |
There was a problem hiding this comment.
When the conversation has fewer than three messages (or the nav is otherwise not rendered), this document-level handler still intercepts Shift+Alt+M and calls preventDefault() even though focusNav() has no target. In that state the shortcut does nothing for LibreChat but can still block the browser/assistive-tech/app shortcut the user was trying to invoke, so check that a nav target exists before consuming the event.
Useful? React with 👍 / 👎.
GitNexus: 🚀 deployedThe |
Round-3 Codex review on #13497: - When a second pointerdown replaces an in-progress drag, run the cleanup with the real drag state so draggingRef is cleared and the rib column resumes auto-centering (was hardcoded to finish(false)). - Only preventDefault on Shift+Alt+M when the nav is actually rendered and has a focus target, so the shortcut no longer swallows browser/AT shortcuts when the nav is absent (<3 messages). focusNav now reports whether it moved focus.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d375a56cf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (draggingRef.current) { | ||
| return; |
There was a problem hiding this comment.
When the indicator column overflows in long conversations, returning here freezes the column while scrollToImmediate is moving the conversation on each pointer move. Because scrubTo maps the pointer against the current button rects in this scrollable column, the drag is effectively limited to the slice of indicators that was visible when the drag started (or however far the pointer can be moved off-column), so users cannot reliably scrub through the rest of a long thread until they release and manually scroll the mini-nav. Consider keeping the column synchronized/autoscrolled during dragging or mapping the pointer to the full entries range instead of freezing this handler.
Useful? React with 👍 / 👎.
| target.focus(); | ||
| return true; |
There was a problem hiding this comment.
On small viewports this nav is still mounted but has hidden md:flex, so target.focus() cannot move focus to a visible control while focusNav() still returns true; the document shortcut handler then calls preventDefault() even though no usable navigation target was focused. This affects conversations with 3+ messages on mobile/tablet widths with a hardware keyboard, and can be avoided by checking that the target is actually focusable/visible before reporting success.
Useful? React with 👍 / 👎.
| document.addEventListener('pointermove', onMove); | ||
| document.addEventListener('pointerup', onUp); | ||
| document.addEventListener('pointercancel', onUp); |
There was a problem hiding this comment.
If the user starts scrubbing and releases after the pointer has left the document/window, these document-level listeners may never receive the matching pointerup/pointercancel, because the column does not capture the pointer or listen for a window blur/lost-capture fallback. In that case draggingRef.current remains true and the auto-centering path keeps returning until another pointerdown or unmount happens, leaving the mini-nav out of sync after the drag. Consider setting pointer capture on pointerdown or adding an explicit global cancellation fallback.
Useful? React with 👍 / 👎.
GitNexus: 🚀 deployedThe |
Round-4 Codex review on #13497: - Map the drag pointer proportionally across the full entries range instead of the visible rib rects, so long conversations whose mini-nav overflows are fully scrubbable in one drag. This is also wobble-immune, so the column auto-centering no longer needs to be frozen mid-drag (removed the freeze and draggingRef). - focusNav now reports success only if focus actually landed, so Shift+Alt+M does not preventDefault when the nav is mounted-but-hidden (hidden md:flex on small viewports). - End the drag if the primary button is released mid-move or the window loses focus, covering pointers released outside the document where pointerup/cancel never arrive.
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
GitNexus: 🚀 deployedThe |
…avila#13497) * 🧭 feat: Add MessageNav Focus Management and Drag-to-Scroll Resolves danny-avila#13491: move keyboard focus into the conversation when a message indicator is selected, and add a Shift+Alt+M shortcut to jump focus back to the nav. Also adds drag-to-scrub interaction across the rib column. * 🖱️ style: Use grab cursor for MessageNav drag affordance * 🐛 fix: Harden MessageNav drag against stale pointer state and dead clicks Addresses Codex/Copilot review on danny-avila#13497: - Ignore pre-drag pointermoves when the primary button is not held, preventing a stale press (released outside the column) from starting a spurious scrub or calling setPointerCapture on an inactive pointer. - Clear the post-drag click-suppression flag after the synthetic-click window so a later activation (including keyboard) is never swallowed. - Match the advertised Shift+Alt+M shortcut by accepting the layout-aware key in addition to the physical code. * 🎯 fix: Track MessageNav drag globally so it survives leaving the column Round-2 Codex review on danny-avila#13497: the 4px threshold was applied before any pointer capture, so a drag that left the narrow rib column before crossing it silently failed to scrub. Replace per-element capture with document-level pointermove/up/cancel listeners attached on pointerdown: - Drag tracking continues regardless of pointer position (fixes diagonal/touch drags off the ribs). - pointerup is always received, so no stale drag state and no setPointerCapture on an inactive pointer (removes the NotFoundError path entirely). - Native click is preserved, so the keyboard/click selection a11y path is unchanged. - Listeners are torn down on pointerup/cancel and on unmount. * 🧹 fix: Reset drag state on pointer replace and gate MessageNav shortcut Round-3 Codex review on danny-avila#13497: - When a second pointerdown replaces an in-progress drag, run the cleanup with the real drag state so draggingRef is cleared and the rib column resumes auto-centering (was hardcoded to finish(false)). - Only preventDefault on Shift+Alt+M when the nav is actually rendered and has a focus target, so the shortcut no longer swallows browser/AT shortcuts when the nav is absent (<3 messages). focusNav now reports whether it moved focus. * ✨ fix: Make MessageNav drag span the whole thread and harden teardown Round-4 Codex review on danny-avila#13497: - Map the drag pointer proportionally across the full entries range instead of the visible rib rects, so long conversations whose mini-nav overflows are fully scrubbable in one drag. This is also wobble-immune, so the column auto-centering no longer needs to be frozen mid-drag (removed the freeze and draggingRef). - focusNav now reports success only if focus actually landed, so Shift+Alt+M does not preventDefault when the nav is mounted-but-hidden (hidden md:flex on small viewports). - End the drag if the primary button is released mid-move or the window loses focus, covering pointers released outside the document where pointerup/cancel never arrive.
…avila#13497) * 🧭 feat: Add MessageNav Focus Management and Drag-to-Scroll Resolves danny-avila#13491: move keyboard focus into the conversation when a message indicator is selected, and add a Shift+Alt+M shortcut to jump focus back to the nav. Also adds drag-to-scrub interaction across the rib column. * 🖱️ style: Use grab cursor for MessageNav drag affordance * 🐛 fix: Harden MessageNav drag against stale pointer state and dead clicks Addresses Codex/Copilot review on danny-avila#13497: - Ignore pre-drag pointermoves when the primary button is not held, preventing a stale press (released outside the column) from starting a spurious scrub or calling setPointerCapture on an inactive pointer. - Clear the post-drag click-suppression flag after the synthetic-click window so a later activation (including keyboard) is never swallowed. - Match the advertised Shift+Alt+M shortcut by accepting the layout-aware key in addition to the physical code. * 🎯 fix: Track MessageNav drag globally so it survives leaving the column Round-2 Codex review on danny-avila#13497: the 4px threshold was applied before any pointer capture, so a drag that left the narrow rib column before crossing it silently failed to scrub. Replace per-element capture with document-level pointermove/up/cancel listeners attached on pointerdown: - Drag tracking continues regardless of pointer position (fixes diagonal/touch drags off the ribs). - pointerup is always received, so no stale drag state and no setPointerCapture on an inactive pointer (removes the NotFoundError path entirely). - Native click is preserved, so the keyboard/click selection a11y path is unchanged. - Listeners are torn down on pointerup/cancel and on unmount. * 🧹 fix: Reset drag state on pointer replace and gate MessageNav shortcut Round-3 Codex review on danny-avila#13497: - When a second pointerdown replaces an in-progress drag, run the cleanup with the real drag state so draggingRef is cleared and the rib column resumes auto-centering (was hardcoded to finish(false)). - Only preventDefault on Shift+Alt+M when the nav is actually rendered and has a focus target, so the shortcut no longer swallows browser/AT shortcuts when the nav is absent (<3 messages). focusNav now reports whether it moved focus. * ✨ fix: Make MessageNav drag span the whole thread and harden teardown Round-4 Codex review on danny-avila#13497: - Map the drag pointer proportionally across the full entries range instead of the visible rib rects, so long conversations whose mini-nav overflows are fully scrubbable in one drag. This is also wobble-immune, so the column auto-centering no longer needs to be frozen mid-drag (removed the freeze and draggingRef). - focusNav now reports success only if focus actually landed, so Shift+Alt+M does not preventDefault when the nav is mounted-but-hidden (hidden md:flex on small viewports). - End the drag if the primary button is released mid-move or the window loses focus, covering pointers released outside the document where pointerup/cancel never arrive. (cherry picked from commit 4dce0fd)
…avila#13497) * 🧭 feat: Add MessageNav Focus Management and Drag-to-Scroll Resolves danny-avila#13491: move keyboard focus into the conversation when a message indicator is selected, and add a Shift+Alt+M shortcut to jump focus back to the nav. Also adds drag-to-scrub interaction across the rib column. * 🖱️ style: Use grab cursor for MessageNav drag affordance * 🐛 fix: Harden MessageNav drag against stale pointer state and dead clicks Addresses Codex/Copilot review on danny-avila#13497: - Ignore pre-drag pointermoves when the primary button is not held, preventing a stale press (released outside the column) from starting a spurious scrub or calling setPointerCapture on an inactive pointer. - Clear the post-drag click-suppression flag after the synthetic-click window so a later activation (including keyboard) is never swallowed. - Match the advertised Shift+Alt+M shortcut by accepting the layout-aware key in addition to the physical code. * 🎯 fix: Track MessageNav drag globally so it survives leaving the column Round-2 Codex review on danny-avila#13497: the 4px threshold was applied before any pointer capture, so a drag that left the narrow rib column before crossing it silently failed to scrub. Replace per-element capture with document-level pointermove/up/cancel listeners attached on pointerdown: - Drag tracking continues regardless of pointer position (fixes diagonal/touch drags off the ribs). - pointerup is always received, so no stale drag state and no setPointerCapture on an inactive pointer (removes the NotFoundError path entirely). - Native click is preserved, so the keyboard/click selection a11y path is unchanged. - Listeners are torn down on pointerup/cancel and on unmount. * 🧹 fix: Reset drag state on pointer replace and gate MessageNav shortcut Round-3 Codex review on danny-avila#13497: - When a second pointerdown replaces an in-progress drag, run the cleanup with the real drag state so draggingRef is cleared and the rib column resumes auto-centering (was hardcoded to finish(false)). - Only preventDefault on Shift+Alt+M when the nav is actually rendered and has a focus target, so the shortcut no longer swallows browser/AT shortcuts when the nav is absent (<3 messages). focusNav now reports whether it moved focus. * ✨ fix: Make MessageNav drag span the whole thread and harden teardown Round-4 Codex review on danny-avila#13497: - Map the drag pointer proportionally across the full entries range instead of the visible rib rects, so long conversations whose mini-nav overflows are fully scrubbable in one drag. This is also wobble-immune, so the column auto-centering no longer needs to be frozen mid-drag (removed the freeze and draggingRef). - focusNav now reports success only if focus actually landed, so Shift+Alt+M does not preventDefault when the nav is mounted-but-hidden (hidden md:flex on small viewports). - End the drag if the primary button is released mid-move or the window loses focus, covering pointers released outside the document where pointerup/cancel never arrive.
Summary
This resolves #13491 by giving the
MessageNavindicator proper keyboard focus management, and additionally makes the rib column draggable so it behaves like an intuitive scrubber instead of a click-only control.MessageNav. The message is made programmatically focusable on demand (tabIndex=-1) and focused withpreventScrollso it doesn't fight the smooth-scroll animation.Shift+Alt+Mshortcut to jump focus back to the nav's current indicator (or the first rib as a fallback), following the existingShift+Alt+Lshortcut convention. Declared viaaria-keyshortcutsso assistive tech announces it.focus:tofocus-visible:(inMessageRender,ContentRender, and nowMessagePartsfor consistency) so the ring appears for keyboard navigation but not for mouse clicks.cursor-ns-resize).computeTargetScrollhelper used by both the eased click-scroll and the immediate drag-scroll.Change Type
Testing
Extended the
MessageNavtest suite with 6 new cases covering focus-on-select, theShift+Alt+Mshortcut (and its modifier guard), drag scrubbing past the threshold, the sub-threshold no-drag case, and post-drag click suppression. Added a minimalPointerEventpolyfill (extending jsdom'sMouseEvent) since jsdom does not implementPointerEvent.To reproduce manually:
MessageNav(3+ messages).Shift+Alt+M— focus should return to the nav's current indicator.Test Configuration:
cd client && npx jest src/components/Chat/Messages/__tests__/MessageNav.spec.tsx— 29/29 pass.Checklist