feat/tab completion#59
Conversation
|
Caution Review failedFailed to post review comments WalkthroughAdds ARCHITECTURE.md and CONTRIBUTING updates; enables testing compose profiles and new bot services; tightens echobot mention behavior; implements tab-based username completion (hook, dropdown, keyboard-shortcuts) with UI integration and supporting tests; adds private-message support (types, store, UI, context menus) and minor build/ignore adjustments. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant CA as ChatArea
participant KS as useKeyboardShortcuts
participant TC as useTabCompletion
participant AD as AutocompleteDropdown
participant IRC as IRC Client
U->>CA: Type text / press Tab
CA->>KS: Keydown event
KS-->>CA: onTabCompletion() if configured
CA->>TC: handleTabCompletion(text, cursor, users)
alt Matches found
TC-->>CA: { newText, newCursor }
CA->>AD: show(matches)
U->>AD: navigate / select
AD-->>CA: onSelect(username)
CA->>TC: confirm selection -> update text/cursor
else No matches
TC-->>CA: null
end
U->>CA: Press Enter
CA->>IRC: sendRaw(message)
sequenceDiagram
autonumber
actor User
participant IRC as IRC Server
participant Echo as echo-bot
User->>IRC: PRIVMSG "#chan" "hello echo-bot" or "echo-bot: ping"
IRC-->>Echo: PRIVMSG delivered
alt Mention detected (whole-word or prefix:)
Echo->>IRC: send typing indicator
Echo->>IRC: PRIVMSG reply "User: you said '...'"
else Private message to bot
Echo->>IRC: send typing indicator
Echo->>IRC: PRIVMSG reply with confirmation
else Not mentioned
Echo-->>IRC: (no response)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
Pre-merge checks and finishing touches and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
✨ 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. Comment |
|
Automated deployment preview for the PR in the Cloudflare Pages. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docker/echobot.py (2)
24-31: Handle socket close to avoid tight loop CPU spin.If
recv()returns empty, the loop spins indefinitely. Break on disconnect.- while True: - data = irc.recv(4096).decode("utf-8", errors="ignore") + while True: + raw = irc.recv(4096) + if not raw: + print("Connection closed by server") + break + data = raw.decode("utf-8", errors="ignore")
37-40: Robust PRIVMSG parsing (limit splits).Unbounded
split(" ")is fragile for IRC trailing params. Limit to 3 splits.- prefix, command, target, *msg_parts = line.split(" ") - sender = prefix.split("!")[0][1:] - message = " ".join(msg_parts)[1:] + parts = line.split(" ", 3) + if len(parts) < 4: + continue + prefix, command, target, trailing = parts + sender = prefix.split("!")[0][1:] + message = trailing[1:] if trailing.startswith(":") else trailing
🧹 Nitpick comments (18)
CONTRIBUTING.md (3)
53-61: Use modern Compose syntax and enable the testing profile explicitly.Prefer “docker compose” (v2) over the deprecated “docker-compose”, and make the testing profile explicit.
Apply this diff:
-# in another terminal -docker-compose --profile testing up -d +# in another terminal +docker compose --profile testing up -d
11-14: “Full stack” instruction is misleading without the testing profile.As written,
docker compose upwon’t start ircd/echo/helper/chat bots because they’re under the “testing” profile. Clarify the command.-Alternatively to run the full ObsidianIRC stack: +Alternatively, to run the full ObsidianIRC stack (including IRCd and bots): ```sh -docker compose up +docker compose --profile testing up--- `24-31`: **Typo: “hoooks”.** Fix spelling in the Git Hooks section. ```diff -We have commit hooks to enforce coding style. You can install the hoooks with: +We have commit hooks to enforce coding style. You can install the hooks with:docker/echobot.py (1)
49-52: Avoid blocking the receive loop with sleep.
time.sleep(2)stalls processing. Consider shorter delay or async typing simulation.- time.sleep(2) # Simulate typing delay + time.sleep(0.5) # Shorter simulated typing delaycompose.yaml (1)
16-30: Add healthcheck to ircd and make bots wait on it.Without readiness gating, bots may connect before Ergo is up, causing flapping.
Example:
ircd: init: true image: ghcr.io/ergochat/ergo container_name: ergo + healthcheck: + test: ["CMD-SHELL", "printf 'PING :health\\r\\n' | timeout 2 nc -w 1 localhost 6667 | grep -q PONG || exit 1"] + interval: 10s + timeout: 3s + retries: 5Then gate bots:
echo-bot: depends_on: - - ircd + ircd: + condition: service_healthy ... helper-bot: depends_on: - - ircd + ircd: + condition: service_healthy ... chat-bot: depends_on: - - ircd + ircd: + condition: service_healthy(Note: ensure
nc/timeoutexist in the image, or replace with a simpler check your base image supports.)ARCHITECTURE.md (3)
8-24: Add language to fenced code block (markdownlint MD040).Specify a language to satisfy linters.
-``` +```text ObsidianIRC/ ├── src/ ...--- `127-134`: **Add language to fenced code block (markdownlint MD040).** Same issue for the test structure snippet. ```diff -``` +```text tests/ ├── setup.ts ...--- `78-83`: **Path casing: align docs with actual file names.** Docs reference `src/lib/ircClient.ts` but build info shows `src/lib/ircclient.ts`. Fix to avoid confusion on case-sensitive filesystems. ```diff -**Location:** `src/lib/ircClient.ts` +**Location:** `src/lib/ircclient.ts`tests/components/ChatArea.test.tsx (2)
255-274: Arrow key caret assertion may be a jsdom artifact.jsdom doesn’t move the caret on arrow keys; this test might pass without proving real behavior. Consider asserting dropdown navigation callbacks instead.
Would you like me to adjust the test to verify
onNavigateeffects rather than caret position?
183-192: Optional: add reverse cycling and Escape tests.Consider tests for Shift+Tab reverse cycling and Escape to close the dropdown.
tests/hooks/useKeyboardShortcuts.test.ts (2)
39-52: Also assert listener cleanup on unmount.Add a test ensuring
removeEventListeneris called when the hook unmounts.it("should add event listener when enabled", () => { const config: KeyboardShortcutsConfig = { shortcuts: [], enabled: true, }; - renderHook(() => useKeyboardShortcuts(config)); + const { unmount } = renderHook(() => useKeyboardShortcuts(config)); expect(document.addEventListener).toHaveBeenCalledWith( "keydown", expect.any(Function), ); + + unmount(); + expect(document.removeEventListener).toHaveBeenCalledWith( + "keydown", + expect.any(Function), + ); });
1-18: Mocking add/removeEventListener globally is fine; consider restoring originals.To reduce cross-test coupling, capture and restore originals in afterEach.
- beforeEach(() => { - document.removeEventListener = vi.fn(); - document.addEventListener = vi.fn(); - }); + const origAdd = document.addEventListener; + const origRemove = document.removeEventListener; + beforeEach(() => { + document.addEventListener = vi.fn() as any; + document.removeEventListener = vi.fn() as any; + }); + afterEach(() => { + document.addEventListener = origAdd; + document.removeEventListener = origRemove; + vi.clearAllMocks(); + });src/components/layout/ChatArea.tsx (3)
94-159: Improve key generation for better React reconciliation.While the key generation has been improved to be more stable, concatenating content in keys can still cause issues if the content contains special characters or is very long. Consider using indices combined with a hash or a more robust approach.
- const partKey = `text-${part}-${index}`; + const partKey = `text-${index}`; const textPart = <span key={partKey}>{part}</span>; // If there's a matching link for this part, render it if (index < matches.length) { - const fragmentKey = `fragment-${matches[index]}-${index}`; + const fragmentKey = `fragment-${index}`;
597-612: Handle selection range API errors gracefully.The
selectionStartproperty might not be supported in all input types or could throw in certain edge cases. Consider adding error handling.const handleInputChange = (e: React.ChangeEvent<HTMLInputElement>) => { const newText = e.target.value; - const newCursorPosition = e.target.selectionStart || 0; + let newCursorPosition = 0; + try { + newCursorPosition = e.target.selectionStart || 0; + } catch (error) { + // Fallback to text length if selectionStart is not available + newCursorPosition = newText.length; + } setMessageText(newText); setCursorPosition(newCursorPosition);
620-683: Consider extracting username completion logic to reduce duplication.The username selection logic is duplicated between the active and inactive tab completion states. This could be extracted into a helper function to improve maintainability.
+ const insertUsername = ( + username: string, + originalText: string, + completionStart: number, + originalPrefix: string, + isAtMessageStart: boolean + ) => { + const suffix = isAtMessageStart ? ": " : " "; + const newText = + originalText.substring(0, completionStart) + + username + + suffix + + originalText.substring(completionStart + originalPrefix.length); + const newCursorPosition = completionStart + username.length + suffix.length; + + setMessageText(newText); + setCursorPosition(newCursorPosition); + + setTimeout(() => { + if (inputRef.current) { + inputRef.current.setSelectionRange( + newCursorPosition, + newCursorPosition, + ); + inputRef.current.focus(); + } + }, 0); + }; const handleUsernameSelect = (username: string) => { if (tabCompletion.isActive) { // Use tab completion state for accurate replacement const isAtMessageStart = tabCompletion.originalText .substring(0, tabCompletion.completionStart) .trim() === tabCompletion.originalPrefix; - const suffix = isAtMessageStart ? ": " : " "; - const newText = - tabCompletion.originalText.substring(0, tabCompletion.completionStart) + - username + - suffix + - tabCompletion.originalText.substring( - tabCompletion.completionStart + tabCompletion.originalPrefix.length, - ); - - setMessageText(newText); - const newCursorPosition = - tabCompletion.completionStart + username.length + suffix.length; - setCursorPosition(newCursorPosition); - - setTimeout(() => { - if (inputRef.current) { - inputRef.current.setSelectionRange( - newCursorPosition, - newCursorPosition, - ); - inputRef.current.focus(); - } - }, 0); + insertUsername( + username, + tabCompletion.originalText, + tabCompletion.completionStart, + tabCompletion.originalPrefix, + isAtMessageStart + ); } else { // Fallback to current logic when tab completion is not active const textBeforeCursor = messageText.substring(0, cursorPosition); const words = textBeforeCursor.split(/\s+/); const currentWord = words[words.length - 1]; const completionStart = cursorPosition - currentWord.length; const isAtMessageStart = textBeforeCursor.trim() === currentWord; - const suffix = isAtMessageStart ? ": " : " "; - const newText = - messageText.substring(0, completionStart) + - username + - suffix + - messageText.substring(cursorPosition); - - setMessageText(newText); - const newCursorPosition = - completionStart + username.length + suffix.length; - setCursorPosition(newCursorPosition); - - setTimeout(() => { - if (inputRef.current) { - inputRef.current.setSelectionRange( - newCursorPosition, - newCursorPosition, - ); - inputRef.current.focus(); - } - }, 0); + insertUsername( + username, + messageText, + completionStart, + currentWord, + isAtMessageStart + ); } setShowAutocomplete(false); tabCompletion.resetCompletion(); };src/components/ui/AutocompleteDropdown.tsx (1)
131-143: Add boundary checks for dropdown positioning.The dropdown positioning doesn't account for viewport boundaries. The dropdown might be positioned outside the visible viewport, especially near the edges of the screen.
const getPosition = () => { if (!inputElement) { return { top: 100, left: 100 }; } const inputRect = inputElement.getBoundingClientRect(); const dropdownHeight = Math.min(displayUsers.length * 48 + 32, 240); + const dropdownWidth = 320; // max-w-xs approximation + + let top = inputRect.top + window.scrollY - dropdownHeight - 8; + let left = inputRect.left + window.scrollX; + + // Ensure dropdown stays within viewport + if (top < 0) { + // Position below input if not enough space above + top = inputRect.bottom + window.scrollY + 8; + } + + if (left + dropdownWidth > window.innerWidth) { + // Align to right edge of viewport if overflowing + left = Math.max(0, window.innerWidth - dropdownWidth - 10); + } return { - top: inputRect.top + window.scrollY - dropdownHeight - 8, - left: inputRect.left + window.scrollX, + top, + left, }; };src/hooks/useKeyboardShortcuts.ts (1)
63-65: Consider documenting the preventDefault default behavior.The condition
shortcut.preventDefault !== falsemeans preventDefault is called by default unless explicitly set to false. This might be unexpected behavior for users of the hook.Consider adding a comment to clarify the default behavior:
+ // preventDefault defaults to true unless explicitly set to false if (shortcut.preventDefault !== false) { event.preventDefault(); }src/hooks/useTabCompletion.ts (1)
84-91: Consider performance optimization for large user lists.For channels with many users, the filtering and sorting operations could impact performance. Consider memoizing the filtered results or limiting the number of matches processed.
const matches = users .map((user) => user.username) .filter( (username) => username.toLowerCase().startsWith(currentWord.toLowerCase()) && username.toLowerCase() !== currentWord.toLowerCase(), ) - .sort((a, b) => a.toLowerCase().localeCompare(b.toLowerCase())); + .sort((a, b) => a.toLowerCase().localeCompare(b.toLowerCase())) + .slice(0, 20); // Limit to first 20 matches for performance
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
ARCHITECTURE.md(1 hunks)CONTRIBUTING.md(1 hunks)README.md(1 hunks)compose.yaml(2 hunks)docker/echobot.py(2 hunks)src/components/layout/ChatArea.tsx(11 hunks)src/components/ui/AutocompleteDropdown.tsx(1 hunks)src/hooks/useKeyboardShortcuts.ts(1 hunks)src/hooks/useTabCompletion.ts(1 hunks)tests/components/ChatArea.test.tsx(1 hunks)tests/hooks/useKeyboardShortcuts.test.ts(1 hunks)tests/hooks/useTabCompletion.test.ts(1 hunks)tsconfig.tsbuildinfo(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
tests/components/ChatArea.test.tsx (3)
src/types/index.ts (3)
User(1-9)Channel(35-46)Server(11-21)src/components/layout/ChatArea.tsx (1)
ChatArea(313-1025)src/lib/ircClient.ts (1)
ircClient(533-533)
tests/hooks/useKeyboardShortcuts.test.ts (1)
src/hooks/useKeyboardShortcuts.ts (3)
KeyboardShortcutsConfig(16-19)useKeyboardShortcuts(26-96)createChatShortcuts(98-112)
src/hooks/useTabCompletion.ts (1)
src/types/index.ts (1)
User(1-9)
src/components/layout/ChatArea.tsx (3)
src/lib/ircUtils.tsx (1)
mircToHtml(167-279)src/hooks/useTabCompletion.ts (1)
useTabCompletion(31-154)src/components/ui/AutocompleteDropdown.tsx (1)
AutocompleteDropdown(18-184)
src/components/ui/AutocompleteDropdown.tsx (1)
src/types/index.ts (1)
User(1-9)
tests/hooks/useTabCompletion.test.ts (2)
src/types/index.ts (1)
User(1-9)src/hooks/useTabCompletion.ts (1)
useTabCompletion(31-154)
🪛 markdownlint-cli2 (0.18.1)
ARCHITECTURE.md
8-8: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
127-127: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (12)
README.md (1)
29-31: LGTM: heading fix is correct.“Contributing” section header change looks good and improves readability.
docker/echobot.py (1)
20-23: Optional: negotiate capabilities for TAGMSG/typing.If Ergo enforces CAPs, request
message-tags/draft/typingbefore sending@+typing=active TAGMSG ....Would you like a small patch to send
CAP REQafter registration?ARCHITECTURE.md (1)
221-224: Verify Node builder version matches Dockerfile.Docs say “Node.js 23 Alpine” for the builder; confirm the Dockerfile uses the same (or prefer LTS).
Want a small PR to align the Dockerfile and docs to Node 22 LTS?
tests/components/ChatArea.test.tsx (3)
92-117: Good coverage of cycling behavior.Tab-cycling across multiple matches and case-insensitive assertions look solid.
235-253: Enter handling with visible dropdown is well validated.Nice guard to ensure normal sending isn’t blocked by the autocomplete UI.
9-15: ircClient default export confirmedsrc/lib/ircClient.ts declares
export const ircClient = new IRCClient()andexport default ircClient(lines ~533–535), so the test's default-export mock is correct.tests/hooks/useKeyboardShortcuts.test.ts (1)
251-284: Solid modifier and case-insensitive matching checks.These assertions exercise the core matching logic well.
tests/hooks/useTabCompletion.test.ts (1)
1-14: LGTM! Well-structured test setup.The test file is properly organized with necessary imports and mock data setup for comprehensive testing of the tab completion functionality.
src/components/layout/ChatArea.tsx (1)
527-562: Good implementation of Tab key handling with autocomplete integration.The tab key handling properly prevents default behavior, integrates with the autocomplete dropdown, and correctly resets tab completion state when appropriate. The flow control logic is well-structured.
src/hooks/useKeyboardShortcuts.ts (1)
1-96: Well-designed keyboard shortcuts hook with proper cleanup.The hook is well-structured with proper event listener management, configuration updates via refs to avoid stale closures, and flexible options for targeting specific elements. The implementation correctly handles modifier keys and propagation control.
src/hooks/useTabCompletion.ts (2)
60-72: Good state validation logic for text changes.The check for unexpected text changes and automatic reset of completion state prevents inconsistent states when the text is modified outside of the tab completion flow. This is a robust approach.
97-99: Clear and correct suffix logic.The logic for determining whether to add a colon (for message start) or just a space (for mid-message) is well-implemented and follows IRC conventions.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
matheusfillipe
force-pushed
the
feat/tab-completion
branch
from
6d81f86 to
e5fd563
Compare
feat/tab completion
3 participants
Summary by CodeRabbit
New Features
Documentation
Tests
Chores