Repository navigation
feat(native-chat): a solid, fading Jump to latest - #25759
Conversation
|
Pullfrog's workflow isn't active on your default branch, so this run didn't start. |
📝 WalkthroughWalkthroughThe native chat message list now uses the Priority: ⬇️ Low Merge Risk: 🔵 Low · up to Keyboard users may lose their visible focus position after jumping to the latest message. Fix the focus handoff before merging, or explicitly accept this bounded accessibility issue. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The inspected navigation changes remain separate from message delivery and approval authority. No new privileged action or weakened approval control was identified. Validation of delayed responses, concurrent actions, and session switching remains limited. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
"Jump to latest" was a faint, see-through pill that popped in and out. It is now a solid button that fades in and out, can't be clicked or tabbed to while hidden, and keeps focus in the composer when clicked. Co-authored-by: Kelvin Amoaba <97001695+AmoabaKelvin@users.noreply.github.com>
121b6e0 to
e16900b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
0506c76b-de15-4acd-85fe-8a073e1b9811
📒 Files selected for processing (3)
src/renderer/src/components/native-chat/NativeChatJumpToLatest.test.tsxsrc/renderer/src/components/native-chat/NativeChatJumpToLatest.tsxsrc/renderer/src/components/native-chat/NativeChatMessageList.tsx
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review.
| export function NativeChatJumpToLatest({ | ||
| visible, | ||
| onJump | ||
| }: { | ||
| visible: boolean | ||
| onJump: () => void | ||
| }): React.JSX.Element { | ||
| const label = translate('components.native-chat.jumpToLatest', 'Jump to latest') | ||
| return ( | ||
| <button | ||
| type="button" | ||
| data-shown={visible} | ||
| inert={!visible} | ||
| aria-hidden={!visible || undefined} | ||
| onMouseDown={keepFocus} | ||
| onClick={onJump} | ||
| className="absolute bottom-3 left-1/2 flex h-8 -translate-x-1/2 items-center gap-1.5 rounded-full border border-border bg-popover px-3 text-xs font-medium text-popover-foreground shadow-floating transition-[opacity,translate] duration-150 ease-out hover:bg-accent hover:text-accent-foreground focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring motion-reduce:transition-none data-[shown=false]:pointer-events-none data-[shown=false]:translate-y-1 data-[shown=false]:opacity-0" | ||
| > | ||
| <ArrowDown className="size-3.5" /> | ||
| <span>{label}</span> | ||
| </button> | ||
| ) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '1,120p' src/renderer/src/components/native-chat/NativeChatJumpToLatest.tsx
sed -n '370,435p' src/renderer/src/components/native-chat/NativeChatMessageList.tsx
cat src/renderer/src/components/native-chat/NativeChatJumpToLatest.test.tsxRepository: stablyai/orca
Length of output: 5715
🏁 Script executed:
git diff --no-ext-diff --unified=50 4de9f9f85a858c711a1513852407354337e1bd36 e16900b2ebfd89afe0aba60336e6787c296ef46d -- src/renderer/src/components/native-chat/NativeChatJumpToLatest.tsx src/renderer/src/components/native-chat/NativeChatMessageList.tsx src/renderer/src/components/native-chat/NativeChatJumpToLatest.test.tsx
printf '\\n--- jump state and handler references ---\\n'
rg -n -C 4 'showJump|jumpToLatest|transcript.*ref|focus\\(' src/renderer/src/components/native-chat/NativeChatMessageList.tsx src/renderer/src/components/native-chat/NativeChatJumpToLatest.tsx src/renderer/src/components/native-chat/NativeChatJumpToLatest.test.tsx
printf '\\n--- focus contract references in native-chat area ---\\n'
rg -n -i -C 2 'focus.*transcript|transcript.*focus|jump.*focus|focus.*jump' src/renderer/src/components/native-chat || test "$?" -eq 1Repository: stablyai/orca
Length of output: 13681
🏁 Script executed:
printf '%s\\n' '--- jump state and handler definitions ---'
rg -n -F -C 8 'showJump' src/renderer/src/components/native-chat/NativeChatMessageList.tsx
rg -n -F -C 8 'jumpToLatest' src/renderer/src/components/native-chat/NativeChatMessageList.tsx src/renderer/src/components/native-chat/use-native-chat-transcript-scroll.ts
printf '%s\\n' '--- scroll hook implementation ---'
sed -n '1,260p' src/renderer/src/components/native-chat/use-native-chat-transcript-scroll.ts
printf '%s\\n' '--- transcript scroll container and focusability ---'
sed -n '345,415p' src/renderer/src/components/native-chat/NativeChatMessageList.tsx
printf '%s\\n' '--- applicable repository focus contract text ---'
rg -n -i -C 2 'focus.*transcript|transcript.*focus|jump.*latest|focus.*restore' docs src/renderer/src/components/native-chat --glob '*.md' --glob '*.tsx' --glob '*.ts' || test "$?" -eq 1Repository: stablyai/orca
Length of output: 37641
🤖 get_repo_knowledge executed:
get_repo_knowledge stablyai/orca /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/learnings /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/conventions
Length of output: 40334
🌐 Web query:
WHATWG HTML inert attribute focused element becomes inert focus behavior activeElement
💡 Result:
**Short answer:** `inert` prevents an element from being focused, but the WHATWG spec does **not** say that setting `inert` on an element that is *already focused* immediately blurs it or moves focus elsewhere.
The spec says inert nodes generally cannot be focused, and excludes inert elements from being focusable areas. A request to focus an inert target is ignored. ([html.spec.whatwg.org](https://html.spec.whatwg.org/dev/interaction.html?utm_source=openai))
So if the currently focused element becomes inert, `document.activeElement` may still report it: the standard’s `activeElement` algorithm reports the document’s focused area, and the spec doesn’t establish a focus change merely because inertness was added. **Inference:** authors should move focus to a non-inert element themselves when making the focused element’s subtree inert; don’t rely on `activeElement` changing automatically. ([html.spec.whatwg.org](https://html.spec.whatwg.org/multipage/interaction.html?utm_source=openai))
Citations:
- 1: https://html.spec.whatwg.org/dev/interaction.html?utm_source=openai
- 2: https://html.spec.whatwg.org/multipage/interaction.html?utm_source=openai
🏁 Script executed:
printf '%s\\n' '--- exact focus-related component lines ---'
nl -ba src/renderer/src/components/native-chat/NativeChatMessageList.tsx | sed -n '160,180p;225,245p;300,312p;365,420p'
printf '%s\\n' '--- exact scroll-hook lines ---'
nl -ba src/renderer/src/components/native-chat/use-native-chat-transcript-scroll.ts | sed -n '85,155p'
printf '%s\\n' '--- test path and focus assertions ---'
nl -ba src/renderer/src/components/native-chat/NativeChatJumpToLatest.test.tsx | sed -n '10,42p'
printf '%s\\n' '--- transcript scroller focus attributes ---'
rg -n -F -C 3 'data-native-chat-scroll' src/renderer/src/components/native-chatRepository: stablyai/orca
Length of output: 26879
Move focus before making the jump button inert.
When a keyboard user activates the button, jumpToLatest calls scrollToBottom, which hides it. The button stays mounted and becomes inert and aria-hidden. HTML does not require inert to move focus from an element that already has focus, so focus can remain on a control hidden from assistive technology. Move focus to the transcript scroll root only when the button has focus. Keep pointer presses from taking focus away from the composer.
Suggested fix
--- a/src/renderer/src/components/native-chat/NativeChatJumpToLatest.tsx
+++ b/src/renderer/src/components/native-chat/NativeChatJumpToLatest.tsx
@@
export function NativeChatJumpToLatest({
visible,
- onJump
+ onJump,
+ buttonRef
}: {
visible: boolean
onJump: () => void
+ buttonRef: React.Ref<HTMLButtonElement>
}): React.JSX.Element {
@@
<button
+ ref={buttonRef}
type="button"
--- a/src/renderer/src/components/native-chat/NativeChatMessageList.tsx
+++ b/src/renderer/src/components/native-chat/NativeChatMessageList.tsx
@@
- const jumpToLatest = useCallback(() => {
+ const jumpButtonRef = useRef<HTMLButtonElement | null>(null)
+ const jumpToLatest = useCallback(() => {
+ if (document.activeElement === jumpButtonRef.current) {
+ scrollRef.current?.focus()
+ }
beginNavigation()
scrollToBottom()
- }, [beginNavigation, scrollToBottom])
+ }, [beginNavigation, scrollRef, scrollToBottom])
@@
<div
ref={scrollRef}
+ tabIndex={-1}
onScroll={onScroll}
@@
- <NativeChatJumpToLatest visible={showJump} onJump={jumpToLatest} />
+ <NativeChatJumpToLatest
+ visible={showJump}
+ onJump={jumpToLatest}
+ buttonRef={jumpButtonRef}
+ />📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function NativeChatJumpToLatest({ | |
| visible, | |
| onJump | |
| }: { | |
| visible: boolean | |
| onJump: () => void | |
| }): React.JSX.Element { | |
| const label = translate('components.native-chat.jumpToLatest', 'Jump to latest') | |
| return ( | |
| <button | |
| type="button" | |
| data-shown={visible} | |
| inert={!visible} | |
| aria-hidden={!visible || undefined} | |
| onMouseDown={keepFocus} | |
| onClick={onJump} | |
| className="absolute bottom-3 left-1/2 flex h-8 -translate-x-1/2 items-center gap-1.5 rounded-full border border-border bg-popover px-3 text-xs font-medium text-popover-foreground shadow-floating transition-[opacity,translate] duration-150 ease-out hover:bg-accent hover:text-accent-foreground focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring motion-reduce:transition-none data-[shown=false]:pointer-events-none data-[shown=false]:translate-y-1 data-[shown=false]:opacity-0" | |
| > | |
| <ArrowDown className="size-3.5" /> | |
| <span>{label}</span> | |
| </button> | |
| ) | |
| } | |
| export function NativeChatJumpToLatest({ | |
| visible, | |
| onJump, | |
| buttonRef | |
| }: { | |
| visible: boolean | |
| onJump: () => void | |
| buttonRef: React.Ref<HTMLButtonElement> | |
| }): React.JSX.Element { | |
| const label = translate('components.native-chat.jumpToLatest', 'Jump to latest') | |
| return ( | |
| <button | |
| ref={buttonRef} | |
| type="button" | |
| data-shown={visible} | |
| inert={!visible} | |
| aria-hidden={!visible || undefined} | |
| onMouseDown={keepFocus} | |
| onClick={onJump} | |
| className="absolute bottom-3 left-1/2 flex h-8 -translate-x-1/2 items-center gap-1.5 rounded-full border border-border bg-popover px-3 text-xs font-medium text-popover-foreground shadow-floating transition-[opacity,translate] duration-150 ease-out hover:bg-accent hover:text-accent-foreground focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring motion-reduce:transition-none data-[shown=false]:pointer-events-none data-[shown=false]:translate-y-1 data-[shown=false]:opacity-0" | |
| > | |
| <ArrowDown className="size-3.5" /> | |
| <span>{label}</span> | |
| </button> | |
| ) | |
| } |
ELI5
In the desktop chat, the "Jump to latest" button that appears when you've scrolled up is a faint, see-through pill that's easy to miss and pops in and out. This makes it a solid, readable button that fades in and out. It's a visual change only: when the button appears and what it does are unchanged.
What Changed
Before: "Jump to latest" was a translucent pill (
bg-card/90, muted text) that appeared and disappeared abruptly. Clicking it moved keyboard focus off the composer, so you had to click back into the message box to keep typing.After:
Mechanism: the button moves out of
NativeChatMessageList.tsxinto a smallNativeChatJumpToLatestcomponent that stays mounted (so it can fade) and isinertwhile hidden. It uses the list's existingshowJumpandjumpToLatest, unchanged.Why
Split out of #24514 so the scrolling behaviour (#24514), this button's look (this PR) and the new "Jump to top" button (#25702) can each be reviewed and landed on their own. This PR is based directly on
mainand doesn't depend on #24514.Linked Issue
Part of #23797.
Visual Proof
Kelvin's recording on #24514 shows this button (it was made before the split): https://github.com/user-attachments/assets/474af3c7-710b-47fa-80c9-1be87206f2dc
Testing
New tests: a mouse press on "Jump to latest" leaves focus in the composer, and a hidden button stays mounted but can't be reached. Each was checked to fail with its behaviour removed. All 21 message-list test files pass, plus the web typecheck, lint and formatting. The look is Kelvin's from #24514.
Notes
Renderer-only. Same on macOS, Linux and Windows. No change for SSH or remote sessions. No new strings.
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)Co-authored by Kelvin (@AmoabaKelvin), who built this in #24514.