feat: rebuild tab description, unified inbox & fuzzy search#475
Draft
felipeggv wants to merge 29 commits intoRunMaestro:mainfrom
Draft
feat: rebuild tab description, unified inbox & fuzzy search#475felipeggv wants to merge 29 commits intoRunMaestro:mainfrom
felipeggv wants to merge 29 commits intoRunMaestro:mainfrom
Conversation
Added optional `description?: string` to AITab for user-defined tab context annotations. This is the data model foundation for the tab description feature. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Extends the Encore Feature system with two new feature flags for upcoming Unified Inbox and Tab Description features. tabDescription defaults to true (enabled by default), unifiedInbox defaults to false. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add missing tests for scoring hierarchy (exact > substring > scattered), case-insensitive scored matching, special characters in query (regex metacharacters, unicode, emoji), and deterministic scoring verification. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…Handlers Adds a new handler for updating AI tab descriptions with trim and empty-string-to-undefined normalization. Follows existing immutable state update pattern via useSessionStore. Includes 4 tests covering set, trim, empty, and whitespace-only cases. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add early returns in fuzzyMatch and fuzzyMatchWithScore for: - Empty text (can't match anything) - Query longer than text (impossible to match all chars) Empty query already had early return. All 5 consumers already skip fuzzy matching on empty input. No consumer changes needed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…gs Encore tab - Import Inbox and FileText icons from lucide-react - Change Encore tab icon from FlaskConical to Sparkles per spec - Add Unified Inbox feature card with toggle, Beta badge, left-border accent - Add Tab Descriptions feature card with toggle, left-border accent - Both cards follow existing toggle pattern (spread-copy + flip flag) - All 113 SettingsModal tests pass, lint clean Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… TabBar Thread the tab description handler from useTabHandlers through useMainPanelProps → App.tsx → MainPanel → TabBar. Gate with encoreFeatures.tabDescription flag (default: false). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…es encore features Add agentInbox shortcut (Alt+I) with encoreFeatures.unifiedInbox guard. Director's Notes shortcut already had its guard. Register agentInbox modal in the modal store and wire through App.tsx keyboard handler context. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Adds a description section to the AI tab hover overlay menu with two modes: - Display mode: FileText icon + description text (2-line clamp) or italic placeholder - Edit mode: auto-focus textarea with Enter to save, Shift+Enter for newline, Escape to cancel Feature-gated behind onUpdateTabDescription prop. Only calls handler when value actually changed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…edit Add cleanup logic so that if the overlay closes while the user is editing a tab description, the current draft is saved rather than discarded. Uses a useRef to track the latest draft value, avoiding stale closure issues. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…n tab overlay Adds requestAnimationFrame-based focus management so that when the user saves or cancels description editing, focus returns to the description display button for keyboard accessibility. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Port 6 inbox files from feat/unified-inbox-encore (which already incorporates all 8 rounds of CodeRabbit/Greptile review fixes) and integrate into the existing codebase with full App.tsx handler wiring. CRITICAL fixes included: - Race condition: effect-driven pendingInboxQuickReply replaces setTimeout - Memory leak: resizeCleanupRef + useEffect unmount guard for drag listeners - Group collision: sessionId as group key, sessionName as display-only label - Ref binding: inboxProcessInputRef.current = processInput after useInputHandlers - Duplicate log: quick reply delegates to processInput (no manual log push) - AI mode: force inputMode='ai' before sending inbox replies - Modal data: openModal preserves data, updateModalData handles undefined base Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…nd unifiedInbox encore feature - Add onOpenAgentInbox gated prop (App.tsx → AppModals → QuickActionsModal) - Add AgentInbox rendering placeholder in App.tsx (commented, component TBD) - Add Unified Inbox entry to QuickActionsModal command palette - DirectorNotesModal gating was already in place from prior work Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Adds 2 missing useTabHandlers tests (non-active session isolation, other tabs isolation) and 7 TabBar description UI tests covering rendering, edit mode, Enter/Escape behavior, and feature gating. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…on consistency The tabDescription encore feature was defaulting to true, inconsistent with the encore pattern (all features disabled by default). No handler or UI exists yet for tab descriptions, so this is purely a default fix. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The reply input auto-focus in FocusModeView used a setTimeout(60ms) fallback alongside requestAnimationFrame. Replaced with double requestAnimationFrame which reliably fires after browser paint without arbitrary timing delays. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ection Adds role="switch" and aria-checked for accessibility compliance. Follows existing Director's Notes toggle pattern. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…gating Add onOpenAgentInbox prop through App.tsx → AppModals → QuickActionsModal chain, gated behind encoreFeatures.unifiedInbox. The action calls the same setAgentInboxOpen(true) used by the Alt+I keyboard shortcut, ensuring all entry points share the same open/close state from modalStore. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…stamp) 26 tests covering: - Filter logic: all/unread/read/starred modes - Sort logic: newest/oldest/grouped/byAgent modes - Summary truncation at MAX_MESSAGE_LENGTH (90 chars) - Timestamp fallback chain: log → createdAt → Date.now() - Exported utilities: truncate() and generateSmartSummary() Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ocus mode) 18 tests across 6 describe blocks covering: - List mode rendering with ARIA attributes - Filter segmented control (All/Unread/Read/Starred) - Keyboard navigation (ArrowUp/Down, Enter to navigate) - Escape/close behavior (overlay click, close button) - Focus mode entry (F key, double-click) and exit (Escape, Backspace) - Reply textarea presence in focus mode Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
updateModalData now stores data even on unopened modals (needed for AgentInbox pattern). Updated test expectation accordingly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…egration # Conflicts: # src/renderer/stores/settingsStore.ts # src/renderer/types/index.ts
…ration # Conflicts: # src/renderer/App.tsx # src/renderer/components/AppModals.tsx # src/renderer/components/QuickActionsModal.tsx # src/renderer/components/SettingsModal.tsx # src/renderer/hooks/keyboard/useMainKeyboardHandler.ts # src/renderer/stores/modalStore.ts # src/renderer/stores/settingsStore.ts # src/renderer/types/index.ts
Added Alt+I (Unified Inbox) and Cmd+Shift+O (Director's Notes) to the Global Shortcuts table in keyboard-shortcuts.md with footnote noting Encore Feature requirement. Also added Unified Inbox to the Available Features table in encore-features.md. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Unified Inbox feature and tab description editing: new AgentInbox UI (list + focus), hook and types, modal/store/shortcut wiring, settings toggles, TabBar description editing and handler, utilities, constants, and many tests across inbox, tab descriptions, modal store, and search scoring. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App as MaestroConsoleInner
participant Store as ModalStore
participant Modal as AgentInboxModal
participant Hook as useAgentInbox
participant Session as SessionStore
User->>App: trigger Unified Inbox (shortcut/menu)
App->>Store: setAgentInboxOpen(true)
Store-->>App: agentInboxOpen = true
App->>Modal: render AgentInbox
Modal->>Hook: request inbox items (sessions, groups, filters)
Hook->>Session: read sessions/groups, generate InboxItem[]
Session-->>Hook: return sessions/groups
Hook-->>Modal: provide sorted/filtered InboxItem[]
Modal->>User: render InboxListView
User->>Modal: select item / enter focus
Modal->>Modal: freeze snapshot & switch to FocusModeView
User->>Modal: submit quick reply
Modal->>App: handleAgentInboxOpenAndReply -> navigate to session & deliver processInput
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Greptile SummaryComprehensive rebuild of three encore features with isolated worktree development: Unified Inbox (cross-agent triage system), Tab Description (editable context notes), and Fuzzy Search (hardened edge cases). Successfully ports CodeRabbit/Greptile fixes including race condition resolution (setTimeout → RAF), resize listener memory leak fix, proper ARIA roles, and O(n*m) → Map optimization. Key improvements:
All features are properly gated behind encore flags, include comprehensive test coverage, and follow existing architectural patterns. Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
Start([User presses Alt+I]) --> OpenInbox[Open AgentInbox Modal]
OpenInbox --> CheckMode{View Mode?}
CheckMode -->|List| ListView[InboxListView]
CheckMode -->|Focus| FocusView[FocusModeView]
ListView --> UseAgentInbox[useAgentInbox Hook]
UseAgentInbox --> FilterSort[Filter & Sort Sessions/Tabs]
FilterSort --> BuildItems[Build InboxItem array]
BuildItems --> VirtualScroll[Virtual Scrolling with react-virtual]
VirtualScroll --> UserAction{User Action}
UserAction -->|F key| EnterFocus[Enter Focus Mode]
UserAction -->|Click item| Navigate[Navigate to Session]
UserAction -->|Change filter| UpdateFilter[Update Filter Mode]
UserAction -->|Esc| CloseModal[Close Modal & Restore Focus]
EnterFocus --> FreezeSnapshot[Freeze Item Order]
FreezeSnapshot --> FocusView
FocusView --> DisplayConvo[Display Conversation Logs]
DisplayConvo --> ShowSidebar[Show Sidebar with Items]
ShowSidebar --> FocusActions{User Action in Focus}
FocusActions -->|Type & Send| QuickReply[Quick Reply Flow]
FocusActions -->|Cmd+Bracket| PrevNext[Navigate Prev/Next Item]
FocusActions -->|Esc or Back| ExitFocus[Exit to List View]
FocusActions -->|Click item| JumpToItem[Jump to Item in Frozen List]
QuickReply --> SetPending[Set pendingInboxQuickReply]
SetPending --> SwitchSession[Switch to Target Session]
SwitchSession --> WaitEffect[useEffect detects active session]
WaitEffect --> CallProcessInput[Call processInput via Ref]
CallProcessInput --> RestoreSession[Restore Previous Session]
Navigate --> CloseAndSwitch[Close Modal & Switch Session]
CloseAndSwitch --> UpdateActiveTab[Update activeTabId & inputMode]
UpdateFilter --> Recompute[Recompute useAgentInbox]
Recompute --> UpdateList[Update Item List]
ExitFocus --> UnfreezeSnapshot[Clear Frozen Order]
UnfreezeSnapshot --> ListView
Last reviewed commit: 2c0b925 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
src/renderer/utils/tabDisplayName.ts (1)
39-39: Consider using "New Agent" for user-facing consistency.Per coding guidelines, user-facing language should use "agent" while "session" is reserved for provider-level conversation contexts. The fallback "New Session" appears in the UI; "New Agent" may align better with the terminology conventions.
This is a minor terminology nit—feel free to keep as-is if there's a specific reason for the current wording.
Based on learnings: "Use the Session interface in code to represent agents (due to historical naming), but use 'agent' in user-facing language and 'session' for provider-level conversation contexts"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/utils/tabDisplayName.ts` at line 39, The fallback user-facing label currently returns "New Session" in tabDisplayName.ts; change this to "New Agent" so UI terminology uses "agent" (while code can keep Session naming). Update the return value in the function that produces the tab title (the fallback branch that currently returns 'New Session') to return 'New Agent' to align with the project's user-facing language guidelines.src/__tests__/renderer/components/TabBar.test.tsx (1)
5784-5791: Remove unnecessaryas anycasts from description tab fixtures.The
AITabinterface already definesdescription?: string(line 429 in src/renderer/types/index.ts), andcreateTabacceptsPartial<AITab>, making the casts at lines 5790 and 5881 redundant. Remove them to maintain type safety in tests.♻️ Proposed cleanup
- } as any), + }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/components/TabBar.test.tsx` around lines 5784 - 5791, Tests are using unnecessary "as any" casts on the createTab fixtures even though AITab defines description?: string and createTab accepts Partial<AITab>; remove the "as any" casts from the createTab calls that pass description (e.g., the fixture in TabBar.test.tsx that constructs tabs with id 'tab-1' and the other occurrence) so the calls use createTab({...}) directly, preserving type safety for AITab and keeping the description as a plain string.src/renderer/stores/modalStore.ts (1)
774-777: TypeupdateAgentInboxDatawithPartial<AgentInboxModalData>instead ofRecord<string, unknown>.Line 777 currently permits arbitrary payload shape, which weakens the modal contract. The
AgentInboxModalDatainterface already exists and matches the call site payload at InboxListView.tsx:517, making this a straightforward improvement to type safety.🔧 Suggested signature tightening
- updateAgentInboxData: (data: Record<string, unknown>) => updateModalData('agentInbox', data), + updateAgentInboxData: (data: Partial<AgentInboxModalData>) => + updateModalData('agentInbox', data),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/stores/modalStore.ts` around lines 774 - 777, Change the updateAgentInboxData setter to accept Partial<AgentInboxModalData> instead of Record<string, unknown> to tighten the modal payload contract: locate the updateAgentInboxData definition in modalStore.ts and replace its parameter type with Partial<AgentInboxModalData>, and if necessary import or reference the AgentInboxModalData type; ensure the call into updateModalData('agentInbox', ...) remains compatible (adjust generics on updateModalData if used).src/renderer/types/agent-inbox.ts (1)
38-44: Add aStatusColorKeyunion type to strengthenSTATUS_COLORStyping.The current
Record<SessionState, string>allows arbitrary strings and would silently fall back to a default color if a typo is introduced. Constraining to a union type catches invalid color keys at compile time.♻️ Suggested typing hardening
+type StatusColorKey = 'success' | 'warning' | 'info' | 'textMuted' | 'error'; + -export const STATUS_COLORS: Record<SessionState, string> = { +export const STATUS_COLORS: Record<SessionState, StatusColorKey> = { idle: 'success', waiting_input: 'warning', busy: 'info', connecting: 'textMuted', error: 'error', };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/types/agent-inbox.ts` around lines 38 - 44, Add a union type (e.g., StatusColorKey = 'success' | 'warning' | 'info' | 'textMuted' | 'error') and use it to tighten STATUS_COLORS typing so it becomes Record<SessionState, StatusColorKey> (or a mapped type SessionState -> StatusColorKey) instead of Record<SessionState, string>; update the export if needed and adjust any code that consumes STATUS_COLORS to expect the new StatusColorKey type. Ensure the union covers the existing values in STATUS_COLORS and update any tests/types that assumed arbitrary strings.src/renderer/components/AgentInbox/InboxListView.tsx (1)
1265-1271: Single-click navigation makes the double-click path effectively unreachable.Line 1269 navigates/closes immediately, so Line 1270’s double-click “enter focus mode” handler won’t realistically execute. Either remove the double-click behavior or make single-click selection-only.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/AgentInbox/InboxListView.tsx` around lines 1265 - 1271, The single-click on InboxItemCardContent currently calls handleNavigate which immediately navigates/ closes the list and prevents the onDoubleClick onEnterFocus handler from firing; change the click behavior so single-click only toggles selection (e.g., call a selection handler like setSelected or onSelectRow instead of handleNavigate) and reserve handleNavigate or onEnterFocus for double-click or explicit open actions, or alternately remove the onDoubleClick prop; update the JSX for InboxItemCardContent to use a selection-only onClick and keep onDoubleClick={() => onEnterFocus(row.item)} (or move navigation to the double-click) so both interactions are reachable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/__tests__/renderer/components/AgentInbox/AgentInbox.test.tsx`:
- Around line 111-126: The test fixture makeSession creates aiTabs via makeTab()
but hardcodes activeTabId to 'default-tab', which can mismatch; update
makeSession to generate the tab first (e.g., const tab = makeTab()), set aiTabs:
[tab], and set activeTabId: tab.id (or fallback to tab.id || 'default-tab') so
activeTabId always matches an existing aiTabs entry; adjust any other places in
makeSession that rely on the previous hardcoded value.
In `@src/renderer/components/AgentInbox/FocusModeView.tsx`:
- Around line 595-597: The reply handlers handleQuickReply and
handleOpenAndReply currently don't call the provided onMarkAsRead callback
despite the comment stating replies should mark messages read; update both
handlers to invoke _onMarkAsRead?.(item.sessionId, item.tabId) at the point
where a reply is performed, and add _onMarkAsRead to each handler's dependency
array so React's memoization picks up the prop change (refer to the
FocusModeView props onMarkAsRead/_onMarkAsRead and the functions
handleQuickReply and handleOpenAndReply).
In `@src/renderer/components/AgentInbox/InboxListView.tsx`:
- Around line 1141-1177: The header button in InboxListView (the button
key={`header-${row.groupKey}`} that calls toggleGroup(row.groupKey)) removes the
default focus outline (outline-none) but doesn't provide any visible focus
replacement, making keyboard focus unclear; restore keyboard accessibility by
adding tabIndex={0}, onFocus/onBlur handlers (e.g., set a local focused state
for that row or call an existing focus handler) and apply a visible focus style
(replace outline-none with a focus style such as a focus ring or visible
border/background change when focused, using isRowSelected or the new focused
state to compute the style) so Enter/Space activation still works and keyboard
focus is visually apparent.
In `@src/renderer/components/AgentInbox/index.tsx`:
- Around line 51-53: The initialization of triggerRef reads
document.activeElement during module/render which can throw in non-DOM
environments; change the useRef initialization to start as null and move the
document.activeElement check into a safe runtime step (e.g., inside a useEffect
or guard with typeof document !== "undefined" or typeof window !== "undefined")
and, if it's an HTMLElement, assign it to triggerRef.current; update references
to triggerRef to rely on the runtime assignment rather than reading document at
module/init time.
In `@src/renderer/components/SettingsModal.tsx`:
- Around line 3687-3690: The new switch buttons in SettingsModal are missing
accessible names; update the button element(s) that use role="switch" (e.g., the
one bound to encoreFeatures.unifiedInbox and the other switch near the same
block) to include an aria-label or aria-labelledby that describes the control
(for example aria-label="Toggle Unified Inbox" or aria-labelledby referencing a
visible label id). Ensure the label text clearly reflects the setting (use the
human-readable name for the feature key, e.g., "Unified Inbox") and keep
consistency for the other switch control(s) in the same component.
In `@src/renderer/components/TabBar.tsx`:
- Around line 763-765: The textarea in TabBar.tsx currently uses an inline ref
callback (ref={(el) => el?.focus()}) which creates a new function each render
and forces focus on every keystroke; replace this behavior by removing the
inline ref callback and using autoFocus on the textarea (or a stable ref with a
one-time focus in a useEffect if explicit focus is required) so editing doesn't
fight the cursor—update the element rendering that references descriptionDraft
to use autoFocus and remove the ref callback to prevent repeated focus calls.
---
Nitpick comments:
In `@src/__tests__/renderer/components/TabBar.test.tsx`:
- Around line 5784-5791: Tests are using unnecessary "as any" casts on the
createTab fixtures even though AITab defines description?: string and createTab
accepts Partial<AITab>; remove the "as any" casts from the createTab calls that
pass description (e.g., the fixture in TabBar.test.tsx that constructs tabs with
id 'tab-1' and the other occurrence) so the calls use createTab({...}) directly,
preserving type safety for AITab and keeping the description as a plain string.
In `@src/renderer/components/AgentInbox/InboxListView.tsx`:
- Around line 1265-1271: The single-click on InboxItemCardContent currently
calls handleNavigate which immediately navigates/ closes the list and prevents
the onDoubleClick onEnterFocus handler from firing; change the click behavior so
single-click only toggles selection (e.g., call a selection handler like
setSelected or onSelectRow instead of handleNavigate) and reserve handleNavigate
or onEnterFocus for double-click or explicit open actions, or alternately remove
the onDoubleClick prop; update the JSX for InboxItemCardContent to use a
selection-only onClick and keep onDoubleClick={() => onEnterFocus(row.item)} (or
move navigation to the double-click) so both interactions are reachable.
In `@src/renderer/stores/modalStore.ts`:
- Around line 774-777: Change the updateAgentInboxData setter to accept
Partial<AgentInboxModalData> instead of Record<string, unknown> to tighten the
modal payload contract: locate the updateAgentInboxData definition in
modalStore.ts and replace its parameter type with Partial<AgentInboxModalData>,
and if necessary import or reference the AgentInboxModalData type; ensure the
call into updateModalData('agentInbox', ...) remains compatible (adjust generics
on updateModalData if used).
In `@src/renderer/types/agent-inbox.ts`:
- Around line 38-44: Add a union type (e.g., StatusColorKey = 'success' |
'warning' | 'info' | 'textMuted' | 'error') and use it to tighten STATUS_COLORS
typing so it becomes Record<SessionState, StatusColorKey> (or a mapped type
SessionState -> StatusColorKey) instead of Record<SessionState, string>; update
the export if needed and adjust any code that consumes STATUS_COLORS to expect
the new StatusColorKey type. Ensure the union covers the existing values in
STATUS_COLORS and update any tests/types that assumed arbitrary strings.
In `@src/renderer/utils/tabDisplayName.ts`:
- Line 39: The fallback user-facing label currently returns "New Session" in
tabDisplayName.ts; change this to "New Agent" so UI terminology uses "agent"
(while code can keep Session naming). Update the return value in the function
that produces the tab title (the fallback branch that currently returns 'New
Session') to return 'New Agent' to align with the project's user-facing language
guidelines.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
docs/encore-features.mddocs/keyboard-shortcuts.mdsrc/__tests__/renderer/components/AgentInbox/AgentInbox.test.tsxsrc/__tests__/renderer/components/TabBar.test.tsxsrc/__tests__/renderer/hooks/useAgentInbox.test.tssrc/__tests__/renderer/hooks/useTabHandlers.test.tssrc/__tests__/renderer/stores/modalStore.test.tssrc/__tests__/renderer/utils/search.test.tssrc/renderer/App.tsxsrc/renderer/components/AgentInbox/FocusModeView.tsxsrc/renderer/components/AgentInbox/InboxListView.tsxsrc/renderer/components/AgentInbox/index.tsxsrc/renderer/components/AppModals.tsxsrc/renderer/components/MainPanel.tsxsrc/renderer/components/QuickActionsModal.tsxsrc/renderer/components/SettingsModal.tsxsrc/renderer/components/TabBar.tsxsrc/renderer/constants/modalPriorities.tssrc/renderer/constants/shortcuts.tssrc/renderer/hooks/keyboard/useMainKeyboardHandler.tssrc/renderer/hooks/props/useMainPanelProps.tssrc/renderer/hooks/tabs/useTabHandlers.tssrc/renderer/hooks/useAgentInbox.tssrc/renderer/stores/modalStore.tssrc/renderer/stores/settingsStore.tssrc/renderer/types/agent-inbox.tssrc/renderer/types/index.tssrc/renderer/utils/search.tssrc/renderer/utils/tabDisplayName.ts
src/__tests__/renderer/components/AgentInbox/AgentInbox.test.tsx
Outdated
Show resolved
Hide resolved
📝 WalkthroughWalkthroughThis PR introduces two new Encore features: Unified Inbox, a virtualized modal inbox for browsing AI agent sessions with list and detail views, and Tab Descriptions, enabling per-AI-tab user-defined descriptions. Implementation includes new components, comprehensive tests, store/hook updates, UI wiring across multiple surfaces, keyboard shortcuts, and documentation updates. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant ModalStore
participant AgentInbox
participant InboxListView
participant QuickActionsModal
User->>QuickActionsModal: Press Alt+I or click "Unified Inbox"
QuickActionsModal->>App: onOpenAgentInbox()
App->>ModalStore: setAgentInboxOpen(true)
ModalStore->>AgentInbox: render with modalData
AgentInbox->>InboxListView: render with items, sortMode, filterMode
InboxListView->>User: display virtualized list
User->>InboxListView: ArrowDown/ArrowUp to navigate
InboxListView->>InboxListView: update selectedIndex
User->>InboxListView: Press Enter or F to focus
InboxListView->>AgentInbox: onNavigateToFocus(selectedIndex)
AgentInbox->>AgentInbox: set viewMode='focus'
AgentInbox->>FocusModeView: render focused item
FocusModeView->>User: display two-pane detail view
sequenceDiagram
participant User
participant TabBar
participant Tab
participant App
participant Store
User->>TabBar: hover/click tab description area
TabBar->>Tab: render description section
Tab->>Tab: isEditingDescription=false, show description/placeholder
User->>Tab: click Edit/Add description button
Tab->>Tab: isEditingDescription=true, descriptionDraft=tab.description
Tab->>User: render textarea with current text
User->>Tab: type new description
Tab->>Tab: update descriptionDraft
User->>Tab: Press Enter or blur
Tab->>Tab: trim descriptionDraft
Tab->>App: onUpdateTabDescription(tabId, trimmed)
App->>Store: updateAITab(tabId, {description: trimmed})
Store->>Tab: tab.description updated
Tab->>Tab: isEditingDescription=false, sync draft
Tab->>User: display updated description
alt User cancels with Escape
User->>Tab: Press Escape
Tab->>Tab: isEditingDescription=false
Tab->>User: revert to original description
end
sequenceDiagram
participant User
participant FocusModeView
participant ReplyArea
participant App
participant Store
participant TargetTab
User->>FocusModeView: view focused inbox item
FocusModeView->>ReplyArea: render reply textarea
User->>ReplyArea: type reply message
ReplyArea->>ReplyArea: update replyText
User->>ReplyArea: Press Cmd+Enter or click Quick Reply
ReplyArea->>App: onQuickReply(reply, targetSession, targetTab)
App->>Store: save reply to message
App->>Store: activate targetSession and targetTab
App->>Store: trigger process commit
alt User presses Shift+Enter for Open & Reply
User->>ReplyArea: Press Shift+Enter
ReplyArea->>App: onOpenAndReply(reply, targetSession, targetTab)
App->>TargetTab: pre-fill input with reply text
App->>TargetTab: navigate to and focus target tab
TargetTab->>User: display reply-ready interface
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (5)
src/renderer/components/SettingsModal.tsx (1)
3687-3690:⚠️ Potential issue | 🟠 MajorAdd accessible names to the new switch controls.
At Line [3688] and Line [3745], the
role="switch"buttons still lack an accessible name, so screen readers announce unlabeled controls.Suggested patch
<button role="switch" + aria-label="Toggle Unified Inbox feature" aria-checked={encoreFeatures.unifiedInbox} onClick={(e) => { @@ <button role="switch" + aria-label="Toggle Tab Descriptions feature" aria-checked={encoreFeatures.tabDescription} onClick={(e) => {Also applies to: 3744-3747
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/SettingsModal.tsx` around lines 3687 - 3690, The switch buttons in SettingsModal (the button with role="switch" that uses aria-checked={encoreFeatures.unifiedInbox} and the other similar switch at the other block) are missing accessible names; add an accessible name by supplying either aria-label or aria-labelledby that references the visible label for each switch (e.g., aria-label="Enable Unified Inbox" or aria-labelledby="{id-of-label}"), keeping aria-checked and onClick logic unchanged so screen readers announce the control and its state; update both the unifiedInbox switch and the other role="switch" instance to use descriptive labels.src/__tests__/renderer/components/AgentInbox/AgentInbox.test.tsx (1)
111-139:⚠️ Potential issue | 🟡 MinorFix fixture mismatch between
aiTabsandactiveTabId.
makeSessioncan produce invalid sessions becauseactiveTabIdis hardcoded (default-tab) whileaiTabsuses generated/overridden IDs. This can make keyboard/focus-mode tests pass for the wrong reason.🐛 Suggested fix
-const makeSession = (overrides: Partial<Session> = {}): Session => - ({ +const makeSession = (overrides: Partial<Session> = {}): Session => { + const aiTabs = (overrides.aiTabs ?? [makeTab()]) as Session['aiTabs']; + const activeTabId = overrides.activeTabId ?? aiTabs[0]?.id ?? 'default-tab'; + return ({ id: `s-${Math.random().toString(36).slice(2, 8)}`, name: 'Agent Alpha', @@ - aiTabs: [makeTab()], - activeTabId: 'default-tab', + aiTabs, + activeTabId, @@ - ...overrides, - }) as unknown as Session; + ...overrides, + aiTabs, + activeTabId, + }) as unknown as Session; +};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/components/AgentInbox/AgentInbox.test.tsx` around lines 111 - 139, The fixture can produce invalid sessions because makeSession sets activeTabId to the hardcoded 'default-tab' while aiTabs contains generated/overridden tab IDs; update makeSession so it creates a tab (via makeTab()), capture its id, set aiTabs to that tab by default, and set activeTabId to either the override's activeTabId if provided or the created tab's id (ensure this logic also respects an overrides.aiTabs array by using its first tab id when present) so activeTabId always matches an id present in aiTabs.src/renderer/components/AgentInbox/index.tsx (1)
51-53:⚠️ Potential issue | 🟡 MinorGuard
document.activeElementaccess during render-time ref initialization.Line 52 reads
document.activeElementunconditionally; this is fragile in non-DOM render/test environments.🛡️ Suggested fix
- const triggerRef = useRef<HTMLElement | null>( - document.activeElement instanceof HTMLElement ? document.activeElement : null - ); + const triggerRef = useRef<HTMLElement | null>( + typeof document !== 'undefined' && document.activeElement instanceof HTMLElement + ? document.activeElement + : null + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/AgentInbox/index.tsx` around lines 51 - 53, The ref initialization reads document.activeElement during render (triggerRef created via useRef) which can fail in non-DOM environments; change the initialization to avoid unconditional access by either (a) checking typeof document !== "undefined" && document.activeElement instanceof HTMLElement before assigning, or (b) initialize triggerRef to null and set triggerRef.current inside a useEffect that runs client-side; update the code around triggerRef/useRef to use one of these guards so document access only occurs in a safe runtime.src/renderer/components/AgentInbox/InboxListView.tsx (1)
1141-1177:⚠️ Potential issue | 🟠 MajorRestore visible keyboard focus on collapsible group headers.
Line 1144 removes the default outline on a focusable control, but there is no replacement focus state.
♿ Suggested fix
<button type="button" key={`header-${row.groupKey}`} className="outline-none" + tabIndex={0} style={{ position: 'absolute', @@ }} onClick={() => toggleGroup(row.groupKey)} + onFocus={(e) => { + e.currentTarget.style.outline = `2px solid ${theme.colors.accent}`; + e.currentTarget.style.outlineOffset = '-2px'; + }} + onBlur={(e) => { + e.currentTarget.style.outline = 'none'; + }} onKeyDown={(e) => { if (e.key === 'Enter' || e.key === ' ') {As per coding guidelines
src/renderer/components/**/*.{ts,tsx}: Add tabIndex attribute and focus event handlers when implementing components that need keyboard focus.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/AgentInbox/InboxListView.tsx` around lines 1141 - 1177, The header button currently removes the default focus outline ("outline-none") but never provides a replacement, so restore visible keyboard focus by making the header truly focusable and rendering a custom focus style: add tabIndex={0} to the button, add onFocus and onBlur handlers (e.g., manage a local isHeaderFocused state) and update the inline style (the button with virtualRow, row.groupKey, isRowSelected and toggleGroup) to show a clear focus indicator (border, boxShadow or outline) when isHeaderFocused is true; alternatively remove the "outline-none" class and use :focus/:focus-visible CSS to show the focus ring, but keep existing onKeyDown/onClick logic intact.src/renderer/components/AgentInbox/FocusModeView.tsx (1)
755-770:⚠️ Potential issue | 🟠 MajorWire mark-as-read into both reply actions.
Line 786 says replies are the explicit read action, but neither handler marks the item read. Replied threads can remain in unread state.
Suggested fix
const handleQuickReply = useCallback(() => { const text = replyText.trim(); if (!text) return; if (onQuickReply) { onQuickReply(item.sessionId, item.tabId, text); } + _onMarkAsRead?.(item.sessionId, item.tabId); setReplyText(''); -}, [replyText, item, onQuickReply]); +}, [replyText, item, onQuickReply, _onMarkAsRead]); const handleOpenAndReply = useCallback(() => { const text = replyText.trim(); if (!text) return; if (onOpenAndReply) { onOpenAndReply(item.sessionId, item.tabId, text); } -}, [replyText, item, onOpenAndReply]); + _onMarkAsRead?.(item.sessionId, item.tabId); +}, [replyText, item, onOpenAndReply, _onMarkAsRead]);#!/bin/bash # Verify both reply handlers invoke _onMarkAsRead and include it in dependencies. rg -n "handleQuickReply|handleOpenAndReply|_onMarkAsRead\\?\\.\\(" src/renderer/components/AgentInbox/FocusModeView.tsx -A10 -B3Also applies to: 786-787
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/AgentInbox/FocusModeView.tsx` around lines 755 - 770, The reply handlers handleQuickReply and handleOpenAndReply currently send replies but do not mark the item read; call the mark-as-read handler (_onMarkAsRead) after a successful reply (e.g., after invoking onQuickReply/onOpenAndReply) to ensure replied threads are marked read, clear reply state where appropriate (setReplyText in handleOpenAndReply if desired), and add _onMarkAsRead to each useCallback dependency array so React sees the change; update handleQuickReply and handleOpenAndReply to invoke _onMarkAsRead(item.sessionId, item.tabId) (or the correct signature) and include _onMarkAsRead in the dependency lists.
🧹 Nitpick comments (2)
src/__tests__/renderer/utils/search.test.ts (1)
434-482: Consolidate overlapping ranking/case test coverage.This block repeats scenarios already validated earlier in this file (ranking relationships and case-score comparisons). Consider collapsing into a parameterized table-driven suite to reduce maintenance noise and keep signal high.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/utils/search.test.ts` around lines 434 - 482, The tests duplicate ranking and case-score comparisons; consolidate the two blocks around fuzzyMatchWithScore into a single parameterized suite using table-driven tests (e.g., test.each / describe.each) that covers: (1) ranking relationships (exact > substring > scattered fuzzy) by supplying pairs/triples of input strings and asserting matches and relative scores, and (2) case-insensitive matching and case-preference (query case matching scores higher) by supplying mixed-case variants and expected match/score relationships; update or remove the existing explicit individual its ("scoring - exact vs fuzzy vs substring ranking" and "case-insensitive matching in scored search") and replace them with the compact parameterized tests that reference fuzzyMatchWithScore so maintenance noise is reduced while preserving all assertions.src/renderer/types/agent-inbox.ts (1)
1-1: PreferToolTypeoverstringfortoolType.Typing
toolTypeasstringdrops compile-time validation and allows invalid values.Suggested patch
-import type { SessionState } from './index'; +import type { SessionState, ToolType } from './index'; @@ - toolType: string; + toolType: ToolType;Also applies to: 10-10
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/types/agent-inbox.ts` at line 1, Change the toolType property from a plain string to the strongly-typed ToolType so consumers get compile-time validation; update the interface/type that defines toolType in this file (and the other occurrences noted) to use ToolType instead of string, and import ToolType from the module that exports it (e.g., add ToolType to the existing import list from './index' or the correct source) so the property signature and all references (toolType) use ToolType.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/App.tsx`:
- Around line 4265-4275: The AgentInbox component is missing the enterToSendAI
prop so it ignores the user's Enter vs Cmd+Enter keyboard preference; update the
AgentInbox JSX (where AgentInbox is rendered) to pass the existing enterToSendAI
state/prop (e.g., enterToSendAI={enterToSendAI}) so AgentInbox receives the
setting and behaves consistently with FocusModeView and other consumers.
In `@src/renderer/components/AgentInbox/FocusModeView.tsx`:
- Around line 988-1005: The resize-handle div in FocusModeView is currently
mouse-only; make it keyboard-accessible by adding tabIndex={0}, an appropriate
role/aria-label (e.g., role="separator" aria-orientation="vertical"
aria-label="Resize sidebar"), and onKeyDown handler that listens for
ArrowLeft/ArrowRight (and optionally Home/End) to call the same resize logic
used by handleResizeStart/resize callbacks (or a new helper that adjusts the
sidebar width), plus onFocus/onBlur to apply the same focus styling used in
onMouseEnter/onMouseLeave; use existing symbols handleResizeStart and
isResizingRef to coordinate state and ensure the visual feedback uses
theme.colors.accent like the mouse interactions.
In `@src/renderer/constants/shortcuts.ts`:
- Line 81: Update the agentInbox shortcut definition so it uses the same
Alt+Meta pattern as other shortcuts: change the keys array for the agentInbox
entry (symbol: agentInbox) from ['Alt','i'] to ['Alt','Meta','i'] to avoid
IME/dead-key conflicts and maintain consistency with existing shortcuts.
In `@src/renderer/stores/modalStore.ts`:
- Around line 775-777: setAgentInboxOpen currently calls
closeModal('agentInbox') which clears the modal data and drops persisted prefs
(filterMode/sortMode/isExpanded); fix by changing closeModal to accept an
optional preserveData boolean (or a mode param) that skips clearing data when
true, then change setAgentInboxOpen to call closeModal('agentInbox', true) on
close so agentInbox preferences are preserved; keep openModal('agentInbox') and
updateModalData('agentInbox', ...) behavior unchanged.
In `@src/renderer/utils/tabDisplayName.ts`:
- Around line 13-15: The current return in the tab display name utility returns
tab.name without trimming, so whitespace-only names like " " appear blank;
update the function in src/renderer/utils/tabDisplayName.ts that checks tab.name
to first do const trimmed = tab.name.trim() and return trimmed if trimmed is
non-empty, otherwise fall back to the existing fallback behavior (i.e., proceed
as if no name was provided).
---
Duplicate comments:
In `@src/__tests__/renderer/components/AgentInbox/AgentInbox.test.tsx`:
- Around line 111-139: The fixture can produce invalid sessions because
makeSession sets activeTabId to the hardcoded 'default-tab' while aiTabs
contains generated/overridden tab IDs; update makeSession so it creates a tab
(via makeTab()), capture its id, set aiTabs to that tab by default, and set
activeTabId to either the override's activeTabId if provided or the created
tab's id (ensure this logic also respects an overrides.aiTabs array by using its
first tab id when present) so activeTabId always matches an id present in
aiTabs.
In `@src/renderer/components/AgentInbox/FocusModeView.tsx`:
- Around line 755-770: The reply handlers handleQuickReply and
handleOpenAndReply currently send replies but do not mark the item read; call
the mark-as-read handler (_onMarkAsRead) after a successful reply (e.g., after
invoking onQuickReply/onOpenAndReply) to ensure replied threads are marked read,
clear reply state where appropriate (setReplyText in handleOpenAndReply if
desired), and add _onMarkAsRead to each useCallback dependency array so React
sees the change; update handleQuickReply and handleOpenAndReply to invoke
_onMarkAsRead(item.sessionId, item.tabId) (or the correct signature) and include
_onMarkAsRead in the dependency lists.
In `@src/renderer/components/AgentInbox/InboxListView.tsx`:
- Around line 1141-1177: The header button currently removes the default focus
outline ("outline-none") but never provides a replacement, so restore visible
keyboard focus by making the header truly focusable and rendering a custom focus
style: add tabIndex={0} to the button, add onFocus and onBlur handlers (e.g.,
manage a local isHeaderFocused state) and update the inline style (the button
with virtualRow, row.groupKey, isRowSelected and toggleGroup) to show a clear
focus indicator (border, boxShadow or outline) when isHeaderFocused is true;
alternatively remove the "outline-none" class and use :focus/:focus-visible CSS
to show the focus ring, but keep existing onKeyDown/onClick logic intact.
In `@src/renderer/components/AgentInbox/index.tsx`:
- Around line 51-53: The ref initialization reads document.activeElement during
render (triggerRef created via useRef) which can fail in non-DOM environments;
change the initialization to avoid unconditional access by either (a) checking
typeof document !== "undefined" && document.activeElement instanceof HTMLElement
before assigning, or (b) initialize triggerRef to null and set
triggerRef.current inside a useEffect that runs client-side; update the code
around triggerRef/useRef to use one of these guards so document access only
occurs in a safe runtime.
In `@src/renderer/components/SettingsModal.tsx`:
- Around line 3687-3690: The switch buttons in SettingsModal (the button with
role="switch" that uses aria-checked={encoreFeatures.unifiedInbox} and the other
similar switch at the other block) are missing accessible names; add an
accessible name by supplying either aria-label or aria-labelledby that
references the visible label for each switch (e.g., aria-label="Enable Unified
Inbox" or aria-labelledby="{id-of-label}"), keeping aria-checked and onClick
logic unchanged so screen readers announce the control and its state; update
both the unifiedInbox switch and the other role="switch" instance to use
descriptive labels.
---
Nitpick comments:
In `@src/__tests__/renderer/utils/search.test.ts`:
- Around line 434-482: The tests duplicate ranking and case-score comparisons;
consolidate the two blocks around fuzzyMatchWithScore into a single
parameterized suite using table-driven tests (e.g., test.each / describe.each)
that covers: (1) ranking relationships (exact > substring > scattered fuzzy) by
supplying pairs/triples of input strings and asserting matches and relative
scores, and (2) case-insensitive matching and case-preference (query case
matching scores higher) by supplying mixed-case variants and expected
match/score relationships; update or remove the existing explicit individual its
("scoring - exact vs fuzzy vs substring ranking" and "case-insensitive matching
in scored search") and replace them with the compact parameterized tests that
reference fuzzyMatchWithScore so maintenance noise is reduced while preserving
all assertions.
In `@src/renderer/types/agent-inbox.ts`:
- Line 1: Change the toolType property from a plain string to the strongly-typed
ToolType so consumers get compile-time validation; update the interface/type
that defines toolType in this file (and the other occurrences noted) to use
ToolType instead of string, and import ToolType from the module that exports it
(e.g., add ToolType to the existing import list from './index' or the correct
source) so the property signature and all references (toolType) use ToolType.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
docs/encore-features.mddocs/keyboard-shortcuts.mdsrc/__tests__/renderer/components/AgentInbox/AgentInbox.test.tsxsrc/__tests__/renderer/components/TabBar.test.tsxsrc/__tests__/renderer/hooks/useAgentInbox.test.tssrc/__tests__/renderer/hooks/useTabHandlers.test.tssrc/__tests__/renderer/stores/modalStore.test.tssrc/__tests__/renderer/utils/search.test.tssrc/renderer/App.tsxsrc/renderer/components/AgentInbox/FocusModeView.tsxsrc/renderer/components/AgentInbox/InboxListView.tsxsrc/renderer/components/AgentInbox/index.tsxsrc/renderer/components/AppModals.tsxsrc/renderer/components/MainPanel.tsxsrc/renderer/components/QuickActionsModal.tsxsrc/renderer/components/SettingsModal.tsxsrc/renderer/components/TabBar.tsxsrc/renderer/constants/modalPriorities.tssrc/renderer/constants/shortcuts.tssrc/renderer/hooks/keyboard/useMainKeyboardHandler.tssrc/renderer/hooks/props/useMainPanelProps.tssrc/renderer/hooks/tabs/useTabHandlers.tssrc/renderer/hooks/useAgentInbox.tssrc/renderer/stores/modalStore.tssrc/renderer/stores/settingsStore.tssrc/renderer/types/agent-inbox.tssrc/renderer/types/index.tssrc/renderer/utils/search.tssrc/renderer/utils/tabDisplayName.ts
Contributor
|
@felipeggv please run the following on your machine for this PR so it can move toward acceptance. Here is the prompt you should run: |
Author
|
@jeffscottward running it. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/__tests__/renderer/components/AgentInbox/AgentInbox.test.tsx (1)
111-114:⚠️ Potential issue | 🟡 Minor
makeSessioncan still generate an invalidactiveTabIdwhenaiTabsis emptyOn Line 113,
activeTabIdfalls back to'default-tab'ifoverrides.aiTabsis[], which can mismatchaiTabsagain.Suggested fix
const makeSession = (overrides: Partial<Session> = {}): Session => { const aiTabs = (overrides.aiTabs ?? [makeTab()]) as Session['aiTabs']; - const activeTabId = overrides.activeTabId ?? aiTabs[0]?.id ?? 'default-tab'; + const activeTabId = overrides.activeTabId ?? aiTabs[0]?.id ?? '';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/components/AgentInbox/AgentInbox.test.tsx` around lines 111 - 114, makeSession can produce an invalid activeTabId because it unconditionally falls back to the string 'default-tab' when overrides.aiTabs is an empty array; change the logic so activeTabId is chosen from overrides.activeTabId if provided, otherwise from the first element of aiTabs (aiTabs[0]?.id), and if aiTabs is empty leave activeTabId undefined (do not use 'default-tab'). Update the makeSession function to compute aiTabs first and then set activeTabId = overrides.activeTabId ?? aiTabs[0]?.id (no hardcoded 'default-tab') so the values remain consistent.src/renderer/components/AgentInbox/InboxListView.tsx (1)
1141-1176:⚠️ Potential issue | 🟡 MinorAdd visible keyboard focus indicator to group header buttons.
The group header button is keyboard-operable but lacks visible focus styling. Keyboard users cannot see which header is focused. Add
onFocus/onBlurhandlers to apply a visible focus ring, consistent with other focusable elements in this component.♿ Suggested fix
<button type="button" key={`header-${row.groupKey}`} style={{ position: 'absolute', // ... existing styles ... }} onClick={() => toggleGroup(row.groupKey)} + onFocus={(e) => { + e.currentTarget.style.outline = `2px solid ${theme.colors.accent}`; + e.currentTarget.style.outlineOffset = '-2px'; + }} + onBlur={(e) => { + e.currentTarget.style.outline = 'none'; + }} onKeyDown={(e) => { if (e.key === 'Enter' || e.key === ' ') {As per coding guidelines: "For focus issues, add
tabIndex={0}ortabIndex={-1}, addoutline-noneclass, and useref={(el) => el?.focus()}for auto-focus on React components."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/AgentInbox/InboxListView.tsx` around lines 1141 - 1176, The group header <button> (the one using virtualRow.start/size and toggleGroup(row.groupKey)) needs visible keyboard focus: add tabIndex={0} and onFocus/onBlur handlers that set a focused state (e.g., isHeaderFocused for that rowKey) or toggle a CSS class, then use that state/class to render a visible focus ring (e.g., outline: 'none' plus a boxShadow or a distinct border color using theme.colors.accent) alongside the existing selected styles; also attach a ref={(el) => el?.focus()} only where auto-focus is required (e.g., when programmatically opening or navigating to a group) so keyboard users can see the focused header. Ensure handlers reference row.groupKey and toggleGroup to keep behavior consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/components/AgentInbox/index.tsx`:
- Around line 224-233: The local Escape handling in handleShellKeyDown conflicts
with the centralized layer stack; remove the 'Escape' case from
handleShellKeyDown so it no longer preventsDefault/stopPropagation or calls
handleExitFocus directly, and ensure this modal is registered with useModalLayer
(with the correct priority in the modal priorities configuration) so the layer
stack becomes the single Escape handler for exiting focus; keep Backspace and
other keys intact but delegate Escape behavior to the modal-layer registration
instead of handling it locally.
---
Duplicate comments:
In `@src/__tests__/renderer/components/AgentInbox/AgentInbox.test.tsx`:
- Around line 111-114: makeSession can produce an invalid activeTabId because it
unconditionally falls back to the string 'default-tab' when overrides.aiTabs is
an empty array; change the logic so activeTabId is chosen from
overrides.activeTabId if provided, otherwise from the first element of aiTabs
(aiTabs[0]?.id), and if aiTabs is empty leave activeTabId undefined (do not use
'default-tab'). Update the makeSession function to compute aiTabs first and then
set activeTabId = overrides.activeTabId ?? aiTabs[0]?.id (no hardcoded
'default-tab') so the values remain consistent.
In `@src/renderer/components/AgentInbox/InboxListView.tsx`:
- Around line 1141-1176: The group header <button> (the one using
virtualRow.start/size and toggleGroup(row.groupKey)) needs visible keyboard
focus: add tabIndex={0} and onFocus/onBlur handlers that set a focused state
(e.g., isHeaderFocused for that rowKey) or toggle a CSS class, then use that
state/class to render a visible focus ring (e.g., outline: 'none' plus a
boxShadow or a distinct border color using theme.colors.accent) alongside the
existing selected styles; also attach a ref={(el) => el?.focus()} only where
auto-focus is required (e.g., when programmatically opening or navigating to a
group) so keyboard users can see the focused header. Ensure handlers reference
row.groupKey and toggleGroup to keep behavior consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 34c6ffdc-dfdc-414b-8dfc-305a8c0663a0
📒 Files selected for processing (10)
src/__tests__/renderer/components/AgentInbox/AgentInbox.test.tsxsrc/renderer/App.tsxsrc/renderer/components/AgentInbox/FocusModeView.tsxsrc/renderer/components/AgentInbox/InboxListView.tsxsrc/renderer/components/AgentInbox/index.tsxsrc/renderer/components/SettingsModal.tsxsrc/renderer/components/TabBar.tsxsrc/renderer/constants/shortcuts.tssrc/renderer/stores/modalStore.tssrc/renderer/utils/tabDisplayName.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/renderer/components/SettingsModal.tsx
- src/renderer/constants/shortcuts.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/renderer/components/AgentInbox/index.tsx (1)
289-290: Consider removing empty event handlers.The
onFocusandonBlurhandlers are no-ops. If they're not needed for preventing event bubbling or future functionality, they can be removed to reduce noise.♻️ Suggested cleanup
onKeyDownCapture={handleCaptureKeyDown} onKeyDown={handleShellKeyDown} - onFocus={() => {}} - onBlur={() => {}} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/AgentInbox/index.tsx` around lines 289 - 290, The onFocus and onBlur no-op handlers on the AgentInbox component are unnecessary noise; remove the empty props (the onFocus={() => {}} and onBlur={() => {}} entries) from the JSX so the element uses default focus behavior. If these were intended to stop event propagation or provide future hooks, either implement the needed logic inside functions (e.g., handleFocus/handleBlur) referenced by the JSX or add a short comment explaining their purpose; otherwise delete the two handlers to clean up the component.src/__tests__/renderer/components/AgentInbox/AgentInbox.test.tsx (1)
391-402: Overlay click test works but could be more robust.The test relies on
dialog.parentElementbeing the clickable overlay. While this matches the current DOM structure, consider adding adata-testid="inbox-overlay"to the outer div for a more resilient selector.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/components/AgentInbox/AgentInbox.test.tsx` around lines 391 - 402, The test is brittle because it assumes the overlay is dialog.parentElement; update the AgentInbox component to add a stable selector (e.g., add data-testid="inbox-overlay" to the outer modal-overlay div) and then update the test in AgentInbox.test.tsx to query the overlay with screen.getByTestId('inbox-overlay') (still fireEvent.click on that element) and assert onClose was called; reference the AgentInbox component's outer div (modal-overlay) and the test's onClose mock to implement this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/__tests__/renderer/components/AgentInbox/AgentInbox.test.tsx`:
- Around line 391-402: The test is brittle because it assumes the overlay is
dialog.parentElement; update the AgentInbox component to add a stable selector
(e.g., add data-testid="inbox-overlay" to the outer modal-overlay div) and then
update the test in AgentInbox.test.tsx to query the overlay with
screen.getByTestId('inbox-overlay') (still fireEvent.click on that element) and
assert onClose was called; reference the AgentInbox component's outer div
(modal-overlay) and the test's onClose mock to implement this change.
In `@src/renderer/components/AgentInbox/index.tsx`:
- Around line 289-290: The onFocus and onBlur no-op handlers on the AgentInbox
component are unnecessary noise; remove the empty props (the onFocus={() => {}}
and onBlur={() => {}} entries) from the JSX so the element uses default focus
behavior. If these were intended to stop event propagation or provide future
hooks, either implement the needed logic inside functions (e.g.,
handleFocus/handleBlur) referenced by the JSX or add a short comment explaining
their purpose; otherwise delete the two handlers to clean up the component.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 42fb6726-8d6c-426a-88a6-e5b7469ef980
📒 Files selected for processing (2)
src/__tests__/renderer/components/AgentInbox/AgentInbox.test.tsxsrc/renderer/components/AgentInbox/index.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/__tests__/renderer/components/AgentInbox/AgentInbox.test.tsx (1)
391-415: Exercise the actual list-mode Escape path here.These tests only cover overlay click and the close button. After moving
Escapehandling intouseModalLayer, a broken layer registration/update would still pass this block while Esc stops closing the modal in production.🧪 Suggested coverage
+ it('Escape in list mode closes via the layer stack', () => { + const onClose = vi.fn(); + const props = createDefaultProps({ onClose }); + render(<AgentInbox {...props} />); + + act(() => { + registeredLayerEscape?.(); + }); + + expect(onClose).toHaveBeenCalledTimes(1); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/components/AgentInbox/AgentInbox.test.tsx` around lines 391 - 415, The tests currently only check overlay click and close-button clicks but do not simulate the Escape key path that relies on useModalLayer registration; add a new test in AgentInbox.test.tsx that renders <AgentInbox .../> in the same list-mode state (use makeTab/makeSession or createDefaultProps as in the other tests), then dispatch an Escape key event (e.g., fireEvent.keyDown(document, { key: 'Escape' }) or userEvent.keyboard('{Escape}')) and assert that the onClose mock passed to AgentInbox was called once; ensure the test targets the AgentInbox component and onClose prop to catch broken useModalLayer registration/update.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/components/AgentInbox/index.tsx`:
- Around line 125-166: When switching in/out of focus mode the selectedIndex
isn't being kept in sync with focusIndex, causing the list to snap back or
selectedIndex to fall out of range; update selectedIndex whenever you change
focusIndex and when exiting focus. Specifically, modify handleNavigateItem and
handleEnterFocus to also call setSelectedIndex(idx) (clamped to items.length-1)
and modify handleExitFocus to setSelectedIndex(focusIndex clamped to current
items.length-1) before clearing frozenOrderRef; use the existing setFocusIndex,
setSelectedIndex, handleEnterFocus, handleNavigateItem, and handleExitFocus
identifiers and ensure any index writes are bounds-checked against items.length.
---
Nitpick comments:
In `@src/__tests__/renderer/components/AgentInbox/AgentInbox.test.tsx`:
- Around line 391-415: The tests currently only check overlay click and
close-button clicks but do not simulate the Escape key path that relies on
useModalLayer registration; add a new test in AgentInbox.test.tsx that renders
<AgentInbox .../> in the same list-mode state (use makeTab/makeSession or
createDefaultProps as in the other tests), then dispatch an Escape key event
(e.g., fireEvent.keyDown(document, { key: 'Escape' }) or
userEvent.keyboard('{Escape}')) and assert that the onClose mock passed to
AgentInbox was called once; ensure the test targets the AgentInbox component and
onClose prop to catch broken useModalLayer registration/update.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 11dbcb36-f6fc-43e5-84e4-c9bd00fa1cdb
📒 Files selected for processing (2)
src/__tests__/renderer/components/AgentInbox/AgentInbox.test.tsxsrc/renderer/components/AgentInbox/index.tsx
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/renderer/components/AgentInbox/InboxListView.tsx (1)
1145-1221: 🛠️ Refactor suggestion | 🟠 MajorRestore visible keyboard focus on collapsible group headers.
The group header
<button>(line 1146) is missingonFocus/onBlurhandlers for visible focus indication. Other interactive elements in this file (e.g.,InboxItemCardContentat lines 139-145,SegmentedControlat lines 389-395) correctly implement focus rings, but this button does not.♿ Suggested fix to add focus handlers
<button type="button" key={`header-${row.groupKey}`} style={{ position: 'absolute', top: 0, left: 0, width: '100%', height: `${virtualRow.size}px`, transform: `translateY(${virtualRow.start}px)`, display: 'flex', alignItems: 'center', paddingLeft: 16, paddingRight: 16, fontSize: 13, fontWeight: 600, color: isRowSelected ? theme.colors.accent : theme.colors.textDim, letterSpacing: '0.5px', textTransform: 'uppercase', borderBottom: `2px solid ${theme.colors.border}40`, borderLeft: isRowSelected ? `3px solid ${theme.colors.accent}` : '3px solid transparent', backgroundColor: isRowSelected ? `${theme.colors.accent}10` : 'transparent', cursor: 'pointer', borderTop: 'none', borderRight: 'none', textAlign: 'left', + outline: 'none', }} onClick={() => toggleGroup(row.groupKey)} + onFocus={(e) => { + e.currentTarget.style.outline = `2px solid ${theme.colors.accent}`; + e.currentTarget.style.outlineOffset = '-2px'; + }} + onBlur={(e) => { + e.currentTarget.style.outline = 'none'; + }} onKeyDown={(e) => { if (e.key === 'Enter' || e.key === ' ') { e.preventDefault(); toggleGroup(row.groupKey); } }} >As per coding guidelines: "For focus issues, add
tabIndex={0}ortabIndex={-1}, addoutline-noneclass, and useref={(el) => el?.focus()}for auto-focus on React components."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/AgentInbox/InboxListView.tsx` around lines 1145 - 1221, The group header button in InboxListView rendering the collapsible group (keyed by row.groupKey) lacks visible focus indication; add keyboard focus support by giving the element tabIndex={0} and adding onFocus and onBlur handlers that toggle a focused state (e.g., local state like isHeaderFocused or reuse existing selection state) to apply a visible focus style (outline or box-shadow) alongside the existing styles, and ensure the handlers call any existing toggleGroup(row.groupKey) only on Enter/Space (leave onClick as-is); update the button rendering in the component that returns the header so focus/blur reliably shows the ring for keyboard users.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/renderer/components/AgentInbox/InboxListView.tsx`:
- Around line 1145-1221: The group header button in InboxListView rendering the
collapsible group (keyed by row.groupKey) lacks visible focus indication; add
keyboard focus support by giving the element tabIndex={0} and adding onFocus and
onBlur handlers that toggle a focused state (e.g., local state like
isHeaderFocused or reuse existing selection state) to apply a visible focus
style (outline or box-shadow) alongside the existing styles, and ensure the
handlers call any existing toggleGroup(row.groupKey) only on Enter/Space (leave
onClick as-is); update the button rendering in the component that returns the
header so focus/blur reliably shows the ring for keyboard users.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 246ab0f4-e48d-428f-bf6a-e16e82679f7e
📒 Files selected for processing (3)
src/__tests__/renderer/components/AgentInbox/AgentInbox.test.tsxsrc/renderer/components/AgentInbox/InboxListView.tsxsrc/renderer/components/AgentInbox/index.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/renderer/components/AgentInbox/index.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Complete rebuild of 3 encore features with isolated worktree development:
CodeRabbit/Greptile Fixes Ported
Test plan
Summary by CodeRabbit
New Features
Documentation
Tests