Minor fixes 0802 #1279
Minor fixes 0802 #1279
Conversation
📝 WalkthroughWalkthroughKeyboard navigation and focus management were added to the event, participant, and tag selection UIs in the desktop app, including new state variables, input refs, and keyboard event handlers. Localization files were updated to reflect shifted line numbers. An additional guideline was added to a template asset regarding markdown formatting. Changes
Sequence Diagram(s)Keyboard Navigation for Event/Participant/Tag SelectionsequenceDiagram
participant User
participant InputField
participant DropdownList
participant AppState
User->>InputField: Focus input
User->>InputField: Type search query
InputField->>DropdownList: Show filtered options
User->>InputField: Press ArrowUp/ArrowDown
InputField->>AppState: Update selectedIndex
AppState->>DropdownList: Highlight selected option
User->>InputField: Press Enter
alt Option selected
AppState->>DropdownList: Trigger selection handler
DropdownList->>InputField: Clear input, close dropdown
InputField->>InputField: Refocus input
else No option selected
InputField->>AppState: Create new entry
AppState->>DropdownList: Update list
end
User->>InputField: Press Escape
InputField->>AppState: Reset selection
AppState->>InputField: Refocus input
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
crates/template/assets/enhance.system.jinja (1)
70-70: Improve clarity and conciseness of the added guideline.The new instruction is repetitive and could be more professional. The existing guidelines already cover this requirement adequately.
Consider this more concise alternative:
-- Never use markdown code block. Just use markdown. Never code block. Never use ```. +- Avoid using markdown code blocks (```). Use plain markdown formatting only.apps/desktop/src/components/editor-area/note-header/chips/event-chip.tsx (1)
423-429: Remove duplicate error handlingThe error is already being logged in the
assignEventmutation definition (line 90). The error handling here is redundant.const handleSelectEvent = async (eventIdToLink: string) => { - assignEvent.mutate(eventIdToLink, { - onError: (error) => { - console.error("Failed to set session event:", error); - }, - }); + assignEvent.mutate(eventIdToLink); };apps/desktop/src/components/editor-area/note-header/chips/participants-chip.tsx (1)
445-456: Consider optimizing the keyboard handler dependenciesThe dependency array is extensive and includes mutation objects that change on every render, potentially causing unnecessary effect re-runs.
Consider using
useCallbackfor the mutation handlers to stabilize the references:+ const handleAddParticipant = useCallback((participant: Human) => { + addParticipantByIdMutation.mutate({ id: participant.id }); + onMutation(); + }, [addParticipantByIdMutation, onMutation]); + + const handleCreateParticipant = useCallback(() => { + addParticipantMutation.mutate({ name: query.trim() }); + onMutation(); + }, [addParticipantMutation, query, onMutation]);Then use these stable callbacks in the keyboard handler instead of inline mutations.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/desktop/src/components/editor-area/note-header/chips/event-chip.tsx(4 hunks)apps/desktop/src/components/editor-area/note-header/chips/participants-chip.tsx(7 hunks)apps/desktop/src/components/editor-area/note-header/chips/tag-chip.tsx(8 hunks)apps/desktop/src/locales/en/messages.po(7 hunks)apps/desktop/src/locales/ko/messages.po(7 hunks)crates/template/assets/enhance.system.jinja(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,ts,tsx,rs}
⚙️ CodeRabbit Configuration File
**/*.{js,ts,tsx,rs}: 1. No error handling.
2. No unused imports, variables, or functions.
3. For comments, keep it minimal. It should be about "Why", not "What".
Files:
apps/desktop/src/components/editor-area/note-header/chips/event-chip.tsxapps/desktop/src/components/editor-area/note-header/chips/tag-chip.tsxapps/desktop/src/components/editor-area/note-header/chips/participants-chip.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: ci (windows, windows-latest)
- GitHub Check: ci (macos, macos-latest)
- GitHub Check: ci
🔇 Additional comments (7)
apps/desktop/src/locales/ko/messages.po (1)
347-347: LGTM! Routine localization metadata updates.The line number references have been correctly updated to reflect source code changes in the related UI components. These are standard maintenance updates required for proper i18n functionality.
Also applies to: 623-623, 903-903, 947-947, 1056-1056, 1251-1251, 1259-1259, 1554-1554
apps/desktop/src/locales/en/messages.po (1)
347-347: LGTM! Routine localization metadata updates.The source code line references have been properly updated to match the recent changes in the UI components. This is standard maintenance for localization files.
Also applies to: 623-623, 903-903, 947-947, 1056-1056, 1251-1251, 1259-1259, 1554-1554
apps/desktop/src/components/editor-area/note-header/chips/event-chip.tsx (2)
310-317: Keyboard navigation state setup looks good!The addition of
selectedIndexfor tracking selection andinputReffor focus management, along with the auto-focus on mount, provides a solid foundation for keyboard accessibility.
470-477: UI selection highlighting implemented correctly!The visual feedback for keyboard navigation using
clsxto conditionally apply the background color provides clear indication of the selected item.apps/desktop/src/components/editor-area/note-header/chips/tag-chip.tsx (2)
350-356: Proper Enter key handling delegation!The conditional handling of Enter key based on selection state prevents conflicts between input submission and dropdown item selection.
396-434: Well-structured keyboard navigation for tag dropdown!The implementation correctly handles navigation between existing tags and the create option, with proper boundary checks and focus management.
apps/desktop/src/components/editor-area/note-header/chips/participants-chip.tsx (1)
509-534: Clean refactoring of ParticipantCandidate!The component properly delegates mutation logic to the parent while maintaining visual selection state. This follows React best practices for controlled components.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
cubic analysis
4 issues found across 6 files • Review in cubic
React with 👍 or 👎 to teach cubic. You can also tag @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| @@ -376,9 +387,57 @@ function TagAddControl({ sessionId, allTags }: { sessionId: string; allTags: { i | |||
|
|
|||
| const handleInputBlur = () => { | |||
| // Delay hiding dropdown to allow clicks on dropdown items | |||
| setTimeout(() => setShowDropdown(false), 200); | |||
| setTimeout(() => { | |||
There was a problem hiding this comment.
The timeout created here is not cleared on component unmount; if the component unmounts within 200 ms, setState will be invoked after unmount, producing React warnings and potential memory leaks. Store the timer id in a ref and clear it in a cleanup function.
Prompt for AI agents
Address the following comment on apps/desktop/src/components/editor-area/note-header/chips/tag-chip.tsx at line 390:
<comment>The timeout created here is not cleared on component unmount; if the component unmounts within 200 ms, setState will be invoked after unmount, producing React warnings and potential memory leaks. Store the timer id in a ref and clear it in a cleanup function.</comment>
<file context>
@@ -376,9 +387,57 @@ function TagAddControl({ sessionId, allTags }: { sessionId: string; allTags: { i
const handleInputBlur = () => {
// Delay hiding dropdown to allow clicks on dropdown items
- setTimeout(() => setShowDropdown(false), 200);
+ setTimeout(() => {
+ setShowDropdown(false);
+ setSelectedIndex(-1);
</file context>
|
|
||
| if (e.key === "ArrowDown") { | ||
| e.preventDefault(); | ||
| setSelectedIndex(selectedIndex < totalDropdownItems - 1 ? selectedIndex + 1 : 0); |
There was a problem hiding this comment.
setSelectedIndex is called using the currently-captured value of selectedIndex; if multiple ArrowDown events fire before React re-renders, the update will be computed from a stale value and some key presses will be ignored. Use the functional updater form so each update is based on the latest state.
Prompt for AI agents
Address the following comment on apps/desktop/src/components/editor-area/note-header/chips/tag-chip.tsx at line 408:
<comment>setSelectedIndex is called using the currently-captured value of selectedIndex; if multiple ArrowDown events fire before React re-renders, the update will be computed from a stale value and some key presses will be ignored. Use the functional updater form so each update is based on the latest state.</comment>
<file context>
@@ -376,9 +387,57 @@ function TagAddControl({ sessionId, allTags }: { sessionId: string; allTags: { i
const handleInputBlur = () => {
// Delay hiding dropdown to allow clicks on dropdown items
- setTimeout(() => setShowDropdown(false), 200);
+ setTimeout(() => {
+ setShowDropdown(false);
+ setSelectedIndex(-1);
+ }, 200);
};
</file context>
| setSelectedIndex(selectedIndex < totalDropdownItems - 1 ? selectedIndex + 1 : 0); | |
| setSelectedIndex(prev => (prev < totalDropdownItems - 1 ? prev + 1 : 0)); |
|
|
||
| if (e.key === "ArrowDown") { | ||
| e.preventDefault(); | ||
| setSelectedIndex(selectedIndex < filteredEvents.length - 1 ? selectedIndex + 1 : 0); |
There was a problem hiding this comment.
setSelectedIndex relies on the stale selectedIndex captured in the closure; use the functional updater to ensure the latest value is used.
Prompt for AI agents
Address the following comment on apps/desktop/src/components/editor-area/note-header/chips/event-chip.tsx at line 397:
<comment>setSelectedIndex relies on the stale selectedIndex captured in the closure; use the functional updater to ensure the latest value is used.</comment>
<file context>
@@ -385,17 +386,64 @@ function EventTab({
return dateB.getTime() - dateA.getTime();
});
+ useEffect(() => {
+ const handleKeyDown = (e: KeyboardEvent) => {
+ if (filteredEvents.length === 0) {
+ return;
+ }
+
</file context>
| setSelectedIndex(selectedIndex < filteredEvents.length - 1 ? selectedIndex + 1 : 0); | |
| setSelectedIndex((prev) => prev < filteredEvents.length - 1 ? prev + 1 : 0); |
| }; | ||
|
|
||
| if (inputRef.current === document.activeElement && totalItems > 0) { | ||
| document.addEventListener("keydown", handleKeyDown); |
There was a problem hiding this comment.
Keyboard listener attached to document may persist after the input loses focus, leading to unexpected global key handling and memory leaks
Prompt for AI agents
Address the following comment on apps/desktop/src/components/editor-area/note-header/chips/participants-chip.tsx at line 442:
<comment>Keyboard listener attached to document may persist after the input loses focus, leading to unexpected global key handling and memory leaks</comment>
<file context>
@@ -372,7 +396,70 @@ function ParticipantCandidates(
}),
});
- const handleClick = () => {
+ const addParticipantByIdMutation = useMutation({
+ mutationFn: ({ id }: { id: string }) => dbCommands.sessionAddParticipant(sessionId, id),
+ onSuccess: () =>
+ queryClient.invalidateQueries({
+ predicate: (query) => (query.queryKey[0] as string).includes("participants") && query.queryKey[1] === sessionId,
</file context>
Summary by cubic
Added keyboard navigation support to event, participant, and tag chips for faster selection and improved accessibility.