modals should focus chat input on exit#111
Conversation
WalkthroughImplements state-driven focus management for the chat input. A new store flag Changes
Sequence DiagramsequenceDiagram
participant QuickActions
participant Store
participant ChatArea
participant InputElement
QuickActions->>QuickActions: User closes quick actions panel
QuickActions->>Store: requestChatInputFocus()
Store->>Store: Set shouldFocusChatInput = true
ChatArea->>Store: Read shouldFocusChatInput (selector)
Store-->>ChatArea: shouldFocusChatInput = true
ChatArea->>ChatArea: useEffect triggered
ChatArea->>InputElement: Focus input element
ChatArea->>Store: clearChatInputFocus()
Store->>Store: Set shouldFocusChatInput = false
Estimated Code Review Effort🎯 2 (Simple) | ⏱️ ~12-18 minutes
Suggested Reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
|
Automated deployment preview for the PR in the Cloudflare Pages. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/components/ChatArea.test.tsx (1)
59-105: ChatArea test store state updated for new UI flagAdding
shouldFocusChatInput: falseto theuisetup keeps this suite in sync with the updatedUIStatedefinition and avoids shape mismatches when the component reads the store.If you want to harden the new behavior, consider a small focused test that sets
shouldFocusChatInputtotruein the store and asserts thatChatAreafocuses the input and clears the flag.src/store/index.ts (1)
370-424: Store-level chat input focus flag and actions look consistent and safeThe new
UIState.shouldFocusChatInputplusrequestChatInputFocus/clearChatInputFocus:
- Are properly typed on
UIStateandAppState.- Are initialized to
falsein the rootuistate.- Use the existing immutable update pattern (
ui: { ...state.ui, ... }), so they won’t clobber other UI fields.- Provide a clean, one-shot “focus request” signal for components like
ChatAreaandQuickActions.This design fits well with the rest of the UI slice and should be easy to extend to other modals if needed.
If you later need per-server or per-tab focus behavior, you could evolve this into a small “focus request” object (e.g.,
{ target: "chatInput", serverId? }) instead of a single boolean, but that’s not necessary for this PR.Also applies to: 676-679, 803-841, 2289-2299
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/components/layout/ChatArea.tsx(1 hunks)src/components/ui/QuickActions.tsx(2 hunks)src/store/index.ts(4 hunks)tests/components/ChatArea.test.tsx(1 hunks)tests/components/MetadataDisplay.test.tsx(1 hunks)tests/lib/nicknameRetry.test.ts(2 hunks)tests/protocol/mode.test.ts(1 hunks)
🔇 Additional comments (7)
tests/protocol/mode.test.ts (1)
25-59: UI fixture correctly extended with new focus flagIncluding
shouldFocusChatInput: falsein the UI slice keeps this test setup aligned with the store’sUIStateand default values. No further changes needed.tests/lib/nicknameRetry.test.ts (1)
35-75: Nickname retry tests correctly updated for UIState changeIncluding
shouldFocusChatInput: falsein both mockeduistates keeps thesePartial<AppState>fixtures compatible with the expandedUIStatewhile remaining behaviorally neutral for the nickname logic under test.Also applies to: 132-172
src/components/ui/QuickActions.tsx (2)
53-53: LGTM! Clean integration with the store.The addition of
requestChatInputFocusfollows the existing pattern for extracting store actions.
59-64: LGTM! Proper focus request on modal close.The implementation correctly requests chat input focus after resetting the modal state, and the dependency array is properly updated.
tests/components/MetadataDisplay.test.tsx (1)
163-163: LGTM! Test updated for new UI state field.The addition of
shouldFocusChatInput: falsecorrectly reflects the new store structure.src/components/layout/ChatArea.tsx (2)
178-181: LGTM! Store selectors correctly extracted.The new selectors
clearChatInputFocusandshouldFocusChatInputare properly extracted from the store.
183-189: Verify mobile platform behavior.The focus effect correctly implements the state-driven focus pattern. However, the existing focus effect at lines 1421-1422 skips auto-focus on mobile platforms (Android/iOS) to avoid disruptive keyboard pop-ups, but this new effect doesn't include that check.
On mobile, auto-focusing when modals close will bring up the keyboard, which may or may not be desired behavior. Please verify this is intentional.
If mobile should be excluded, apply this diff:
// Focus chat input when requested by other components (e.g., modals closing) useEffect(() => { + // Skip auto-focus on mobile platforms + if ("__TAURI__" in window && ["android", "ios"].includes(platform())) + return; + if (shouldFocusChatInput && inputRef.current) { inputRef.current.focus(); clearChatInputFocus(); } }, [shouldFocusChatInput, clearChatInputFocus]);
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.