feat: implement resizable split pane functionality #40
Conversation
- Add react-resizable-panels for side-by-side chat viewing - Implement drag-and-drop from navbar tabs to split zone - Add split pane state management in ChatProvider - Create SplitPaneChat component for split panel content - Add close button functionality for removable panels - Support flexible panel management where any panel can be closed - Include proper minimum panel sizes for usable interface - Fix chat input width for split panel layouts The feature allows users to drag chatroom tabs onto a split zone icon to create resizable panels for viewing multiple chats simultaneously. Panels can be closed individually with automatic panel promotion when the main panel is closed.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds split-pane, resizable chat panels with drag-to-split support, a new SplitPaneChat component, store APIs and MAX_SPLIT_PANE_COUNT, migration from hello-pangea DnD to Atlassian pragmatic DnD, multiple SCSS updates, Mentions/StreamerInfo close controls, event-collector reconnection/sampling enhancements, dependency updates, and an Architecture-Overview doc. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant NB as Navbar (pragmatic DnD)
participant DZ as Split Zone
participant ST as Chat Store (Zustand)
participant CP as Chat Page (Panels)
U->>NB: Drag chatroom tab
NB->>DZ: Hover over split-zone (isDraggingOverSplit = true)
U->>DZ: Drop chatroom on split-zone
DZ-->>NB: drop accepted
NB->>ST: addToSplitPane(chatroomId)
ST-->>CP: splitPaneChatrooms updated
CP->>CP: Render new Panel with SplitPaneChat
sequenceDiagram
autonumber
participant U as User
participant H as Header (StreamerInfo / Mentions)
participant CP as Chat Page
participant ST as Chat Store
U->>H: Click close (X)
H-->>CP: onClose(chatroomId)
alt Closing split-pane panel
CP->>ST: removeFromSplitPane(chatroomId)
ST-->>CP: splitPaneChatrooms updated
CP->>CP: Remove panel
else Closing main panel while split panes exist
CP->>ST: removeFromSplitPane(firstSplitId)
CP->>CP: Promote firstSplitId to main panel and re-render
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codex Review: Here are some suggestions.
Reply with @codex fix comments to fix any unresolved comments.
About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (24)
src/renderer/src/providers/ChatProvider.jsx (1)
400-401: Split-pane state added — consider persistence and type normalization.Today this resets on reload and may mix "123" vs 123. Optional: persist to localStorage and normalize IDs to a single type.
src/renderer/src/assets/styles/pages/ChatPage.scss (2)
195-202: Add focus-visible styles for keyboard users.Provide a clear focus state for the close button.
.splitPaneCloseBtn { opacity: 0.5; transition: opacity 0.2s ease; + &:focus-visible { + outline: 2px solid var(--accent-color); + outline-offset: 2px; + opacity: 1; + } &:hover { opacity: 1; } }
845-875: Improve handle UX: selection/touch behavior and focus state.Prevent accidental text selection/scroll hijack and add a keyboard focus style.
.resize-handle { width: 4px; background: var(--border-primary); cursor: col-resize; position: relative; transition: background-color 0.2s ease-in-out; + user-select: none; + -webkit-user-select: none; + touch-action: none; + &:focus-visible { + outline: 2px solid var(--accent-color); + outline-offset: 2px; + }src/renderer/src/assets/styles/components/SplitPaneChat.scss (1)
57-79: Make the close control accessible.Add a visible focus ring for keyboard navigation.
.splitPaneChatClose { display: flex; align-items: center; justify-content: center; width: 28px; height: 28px; border: none; border-radius: 4px; background: transparent; color: var(--text-muted); cursor: pointer; transition: all 0.2s ease-in-out; flex-shrink: 0; + &:focus-visible { + outline: 2px solid var(--accent-color); + outline-offset: 2px; + color: var(--text-primary); + }docs/Architecture-Overview.md (2)
35-58: Fix markdown lint: add language to code fence.Annotate fenced block to satisfy MD040.
-``` +```text App ├── ErrorBoundary ... └── SplitPaneChat -``` +```
195-226: Fix markdown lint: add language to code fence.Annotate fenced block to satisfy MD040.
-``` +```text src/ ├── main/ # Main process (Node.js) ... └── performance-budget.js # Performance monitoring -``` +```src/renderer/src/assets/styles/components/Navbar.scss (2)
86-121: Two drop-zone style stacks diverge; consider a base + modifier.You define splitPaneDropZone(Container) here (40×40) and again globally (32×80). Consolidate to a base class with size modifiers to avoid drift.
663-706: Global drop-zone duplication with different sizing.Introduce BEM-like modifiers (e.g., .splitPaneDropZone--compact/.--wide) and share common rules to prevent inconsistencies.
-.splitPaneDropZone { /* global */ +.splitPaneDropZone { /* base */ /* shared styles */ } +.splitPaneDropZone--compact { height: 32px; width: 80px; padding: 1px 16px; } +/* keep the 40x40 variant under navbar scope or add .splitPaneDropZone--square */src/renderer/src/components/Chat/index.jsx (1)
14-14: Default the new props to safe values.Prevents accidental undefined handler calls if a caller omits them.
-const Chat = ({ chatroomId, kickUsername, kickId, settings, updateSettings, showCloseButton, onClose }) => { +const Chat = ({ chatroomId, kickUsername, kickId, settings, updateSettings, showCloseButton = false, onClose = () => {} }) => {src/renderer/src/components/Chat/StreamerInfo.jsx (3)
20-20: Default and validate new props (showCloseButton,onClose).Guard against
onClosebeing undefined and make intent explicit.- ({ streamerData, isStreamerLive, chatroomId, userChatroomInfo, settings, updateSettings, handleSearch, showCloseButton, onClose }) => { + ({ streamerData, isStreamerLive, chatroomId, userChatroomInfo, settings, updateSettings, handleSearch, showCloseButton = false, onClose = () => {} }) => {
167-174: Close button: add button type and keep click safe.Prevents accidental form submission in nested forms and relies on the defaulted
onClose.- {showCloseButton && ( - <button - className="splitPaneCloseBtn" - onClick={onClose} - aria-label="Close split pane"> + {showCloseButton && ( + <button + type="button" + className="splitPaneCloseBtn" + onClick={onClose} + aria-label="Close split pane"> <XIcon size={20} /> </button> )}
291-299: Memo comparator references props not passed to ChattersBtn.
isStreamerLiveanduserChatroomInfoaren’t props here; remove from comparator to avoid confusion.(prevProps, nextProps) => { return ( prevProps.chatroomId === nextProps.chatroomId && - prevProps.streamerData === nextProps.streamerData && - prevProps.isStreamerLive === nextProps.isStreamerLive && - prevProps.userChatroomInfo === nextProps.userChatroomInfo + prevProps.streamerData === nextProps.streamerData ); },src/renderer/src/components/SplitPaneChat.jsx (2)
6-6: DefaultonCloseprop.Avoids undefined handler edge cases.
-const SplitPaneChat = ({ chatroomId, kickUsername, kickId, settings, updateSettings, onClose }) => { +const SplitPaneChat = ({ chatroomId, kickUsername, kickId, settings, updateSettings, onClose = () => {} }) => {
14-16: Close button: add type attribute.Prevents accidental form submission in nested forms.
- <button className="splitPaneChatClose" onClick={onClose} aria-label="Close split pane"> + <button type="button" className="splitPaneChatClose" onClick={onClose} aria-label="Close split pane">src/renderer/src/components/Navbar.jsx (8)
8-8: Remove unused import (Draggable) in this file.
Draggableis used inside ChatroomTab, not here.-import { DragDropContext, Droppable, Draggable } from "@hello-pangea/dnd"; +import { DragDropContext, Droppable } from "@hello-pangea/dnd";
34-34: Drop unused local state.
isDraggingOverSplitisn’t used in rendering.- const [isDraggingOverSplit, setIsDraggingOverSplit] = useState(false); + // (removed) local drag-over state was unused
40-40: Drop unused ref.
splitZoneRefis assigned but never read.- const splitZoneRef = useRef(null); + // (removed) unused ref
101-113: Remove no-op drag start/update handlers.They only toggled removed state; simplify DnD wiring.
- const handleDragStart = () => { - setIsDraggingOverSplit(false); - }; - - const handleDragUpdate = (update) => { - // Check if hovering over split zone droppable - if (update.destination?.droppableId === 'split-zone') { - setIsDraggingOverSplit(true); - } else { - setIsDraggingOverSplit(false); - } - }; + // (removed) no-op drag start/update handlers
115-116: Tidy drag end reset.Remove reset of deleted state.
- setIsDraggingOverSplit(false); -
249-254: Simplify DragDropContext props.Drop unused handlers after cleanup.
- <DragDropContext - onDragStart={handleDragStart} - onDragUpdate={handleDragUpdate} - onDragEnd={handleDragEnd} - > + <DragDropContext onDragEnd={handleDragEnd}>
316-321: Use provided ref directly; remove hidden placeholder wrapper.Reduces indirection and avoids odd layout behaviors.
- ref={(el) => { - provided.innerRef(el); - splitZoneRef.current = el; - }} + ref={provided.innerRef} {...provided.droppableProps} className={clsx("splitPaneDropZone", snapshot.isDraggingOver && "active")} > - <span>Split</span> - <SquareSplitHorizontalIcon weight="bold" size={16} aria-label="Split pane" /> - <div style={{ display: 'none' }}>{provided.placeholder}</div> + <span>Split</span> + <SquareSplitHorizontalIcon weight="bold" size={16} aria-label="Split pane" /> + {provided.placeholder}
124-129: Guard against stale indices when dropping to split-zone.If the source list mutates mid-drag, using
source.indexcould be unsafe. Resolve by ID fromresult.draggableId.- if (destination.droppableId === 'split-zone') { - const draggedChatroom = orderedChatrooms[source.index]; - addToSplitPane(draggedChatroom.id); + if (destination.droppableId === 'split-zone') { + const draggedId = result.draggableId.replace(/^item-/, ''); + const dragged = orderedChatrooms.find(c => String(c.id) === draggedId) || orderedChatrooms[source.index]; + if (dragged) addToSplitPane(dragged.id); return; }Run quick checks that
draggableIdformat matchesChatroomTab’sdraggableId. If different, adjust the regex.src/renderer/src/pages/ChatPage.jsx (2)
89-96: Main-pane close behavior: confirm UX.Closing the main pane promotes the first split pane. This keeps the chatrooms list intact (no removal). If the intent was “close main view only,” this is correct; if it was “close chatroom,” wire to
removeChatroom.
160-197: Persist and stabilize panel layout — autoSaveId is supportedpackage.json lists react-resizable-panels as ^3.0.6 and that library’s PanelGroup supports autoSaveId (persists to localStorage) and also provides onLayout + storage for custom/SSR strategies. (github.com)
- <PanelGroup direction="horizontal" className="chatPanels"> + <PanelGroup + direction="horizontal" + className="chatPanels" + autoSaveId="chat-panels" + // Optional: force re-layout when split count changes + key={`panes-${splitPaneChatrooms.length}`} + >If you need SSR-safe restoration or a different storage backend, use PanelGroup's onLayout and storage props (or persist server-side and pass default sizes). (github.com)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
docs/Architecture-Overview.md(1 hunks)package.json(1 hunks)src/renderer/src/assets/styles/components/Chat/Input.scss(1 hunks)src/renderer/src/assets/styles/components/Navbar.scss(4 hunks)src/renderer/src/assets/styles/components/SplitPaneChat.scss(1 hunks)src/renderer/src/assets/styles/pages/ChatPage.scss(2 hunks)src/renderer/src/components/Chat/StreamerInfo.jsx(3 hunks)src/renderer/src/components/Chat/index.jsx(2 hunks)src/renderer/src/components/Navbar.jsx(4 hunks)src/renderer/src/components/SplitPaneChat.jsx(1 hunks)src/renderer/src/pages/ChatPage.jsx(3 hunks)src/renderer/src/providers/ChatProvider.jsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/renderer/src/components/SplitPaneChat.jsx (2)
src/renderer/src/components/Chat/index.jsx (2)
chatroom(17-17)Chat(14-100)src/renderer/src/providers/ChatProvider.jsx (2)
useChatStore(408-3914)settings(3923-3923)
src/renderer/src/pages/ChatPage.jsx (3)
src/renderer/src/providers/ChatProvider.jsx (2)
useChatStore(408-3914)settings(3923-3923)src/renderer/src/components/Chat/index.jsx (1)
Chat(14-100)src/renderer/src/components/SplitPaneChat.jsx (1)
SplitPaneChat(6-42)
src/renderer/src/components/Navbar.jsx (3)
src/renderer/src/providers/ChatProvider.jsx (3)
useChatStore(408-3914)settings(3923-3923)span(56-56)src/renderer/src/components/Navbar/ChatroomTab.jsx (1)
ChatroomTab(15-190)src/renderer/src/components/Navbar/MentionsTab.jsx (1)
MentionsTab(5-34)
🪛 markdownlint-cli2 (0.17.2)
docs/Architecture-Overview.md
35-35: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
195-195: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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). (1)
- GitHub Check: test-builds (windows-latest)
🔇 Additional comments (4)
package.json (1)
96-96: New dep: confirm compatibility and bundle footprint.react-resizable-panels ^3.0.6 should work with React 18 and Chromium/Electron. Please sanity‑check: (a) no SSR-only code paths, (b) package size impact, (c) peer deps match. Consider lockfile update and SBOM scan before release.
src/renderer/src/assets/styles/components/Chat/Input.scss (1)
15-15: Good fix: container-relative width.Switching to calc(100% - 16px) aligns the input with split panes.
src/renderer/src/components/Chat/index.jsx (1)
76-78: Prop pass-through LGTM.Forwarding showCloseButton/onClose to StreamerInfo aligns with SplitPaneChat usage.
src/renderer/src/pages/ChatPage.jsx (1)
162-171: A11y: close affordance only in Chat, not Mentions.When Mentions is active, there’s no close affordance; confirm that’s intentional.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/renderer/src/pages/ChatPage.jsx (1)
90-97: Split-pane cap + default/min sizes address earlier crash.Capping via MAX_SPLIT_PANE_COUNT and sensible defaults/mins avoids MinSizesNotSatisfiable.
Also applies to: 169-213
src/renderer/src/providers/ChatProvider.jsx (1)
2279-2281: Nice: orphan split-pane cleanup on chatroom removalFiltering splitPaneChatrooms here prevents empty panes after removeChatroom. This addresses prior feedback.
🧹 Nitpick comments (13)
src/renderer/src/assets/styles/dialogs/mentions.scss (1)
54-75: Add keyboard focus styles and limit transitioned properties.Improve a11y and avoid expensive “all” transitions.
Apply:
.mentionsGlobalActionBtn { - transition: all 0.2s; + transition: background 0.2s, color 0.2s; + + &:focus-visible { + outline: 2px solid rgba(255, 255, 255, 0.7); + outline-offset: 2px; + } }docs/Architecture-Overview.md (1)
1-1: Add a “Last updated” marker.Helps readers assess staleness.
-# KickTalk Architecture Overview +# KickTalk Architecture Overview +<sub>Last updated: 2025-09-18</sub>src/renderer/src/components/Dialogs/Mentions.jsx (2)
70-72: Fix 24h/AM-PM timestamp mix.Use 12h with AM/PM.
- return dayjs(timestamp).format("HH:mm A"); + return dayjs(timestamp).format("h:mm A");
45-49: Complete memo deps for stability.Include getAllMentions to avoid stale closures if store selectors change.
-}, [selectedChatroom, mentions, getChatroomMentions]); +}, [selectedChatroom, mentions, getChatroomMentions, getAllMentions]);src/renderer/src/pages/ChatPage.jsx (2)
12-43: Gate periodic telemetry to dev builds.Avoids prod overhead from 10s interval.
- useEffect(() => { + useEffect(() => { + if (!import.meta.env?.DEV) return;
183-189: Route Mentions navigation through handleChatroomSwitch to keep telemetry.Directly setting state skips span/metrics.
- <Mentions - setActiveChatroom={setActiveChatroomId} + <Mentions + setActiveChatroom={handleChatroomSwitch} chatroomId={activeChatroomId} showCloseButton={splitPaneChatrooms.length > 0} onClose={handleCloseMainPanel} />src/renderer/src/components/Navbar.jsx (4)
9-9: Remove unused Draggable import.-import { DragDropContext, Droppable, Draggable } from "@hello-pangea/dnd"; +import { DragDropContext, Droppable } from "@hello-pangea/dnd";
37-44: Drop unused drag-over state/ref and simplify DnD handlers.Reduces noise; behavior unchanged.
-const [isDraggingOverSplit, setIsDraggingOverSplit] = useState(false); -const splitZoneRef = useRef(null); - -const handleDragStart = () => { - setIsDraggingOverSplit(false); -}; - -const handleDragUpdate = (update) => { - if (update.destination?.droppableId === 'split-zone' && canAcceptMoreSplitPanes) { - setIsDraggingOverSplit(true); - } else { - setIsDraggingOverSplit(false); - } -}; +// (removed) -<DragDropContext - onDragStart={handleDragStart} - onDragUpdate={handleDragUpdate} - onDragEnd={handleDragEnd} -> +<DragDropContext onDragEnd={handleDragEnd}> - setIsDraggingOverSplit(false); + // (removed) - ref={(el) => { - provided.innerRef(el); - splitZoneRef.current = el; - }} + ref={provided.innerRef}Also applies to: 103-115, 253-256, 117-118, 320-324
331-334: Don’t hide the Droppable placeholder.Let the lib manage spacing; hiding can impact measurements.
- <div style={{ display: 'none' }}>{provided.placeholder}</div> + {provided.placeholder}
314-344: Offer a non–drag-and-drop path (a11y).Consider adding “Open in split pane” to the ChatroomTab context menu to support keyboard-only users.
If you want, I can propose a small patch to ChatroomTab to add this menu item wired to addToSplitPane().
src/renderer/src/providers/ChatProvider.jsx (3)
404-418: Harden ID normalization to reject malformed numeric stringsparseInt will coerce "12abc" → 12. Prefer a strict integer check to avoid accidental matches; also guard against non-positive values.
Apply this diff:
-export const MAX_SPLIT_PANE_COUNT = 3; +export const MAX_SPLIT_PANE_COUNT = 3; -const normalizeChatroomIdForSplitPane = (rawId) => { - if (rawId == null) { - return null; - } - - if (typeof rawId === 'number') { - return Number.isFinite(rawId) ? rawId : null; - } - - const parsed = parseInt(String(rawId), 10); - return Number.isNaN(parsed) ? null : parsed; -}; +const normalizeChatroomIdForSplitPane = (rawId) => { + if (rawId == null) return null; + if (typeof rawId === 'number') return Number.isSafeInteger(rawId) && rawId > 0 ? rawId : null; + const s = String(rawId).trim(); + if (!/^\d+$/.test(s)) return null; + const n = Number(s); + return Number.isSafeInteger(n) && n > 0 ? n : null; +};
2313-2350: Return success/failure from addToSplitPane for better UX hooksLet callers show toasts or branch on limit/duplicate/unknown-room without re-reading state.
Apply this diff:
- addToSplitPane: (chatroomId) => { + addToSplitPane: (chatroomId) => { + let ok = false; const normalizedId = normalizeChatroomIdForSplitPane(chatroomId); if (normalizedId == null) { console.warn('[SplitPane] Ignoring invalid chatroom id for split pane:', chatroomId); - return; + return false; } set((state) => { const currentSplitPanes = state.splitPaneChatrooms || []; if (currentSplitPanes.length >= MAX_SPLIT_PANE_COUNT) { window.app?.telemetry?.recordEvent?.('split_pane_limit_reached', { attemptedChatroomId: String(chatroomId), limit: MAX_SPLIT_PANE_COUNT, }); - return state; + return state; } const targetRoom = state.chatrooms.find((room) => safeChatroomIdMatch(room.id, normalizedId, 'addToSplitPane:lookup'), ); if (!targetRoom) { console.warn('[SplitPane] Tried to add unknown chatroom to split pane:', chatroomId); - return state; + return state; } const alreadySplit = currentSplitPanes.some((id) => safeChatroomIdMatch(id, targetRoom.id, 'addToSplitPane:duplicate'), ); if (alreadySplit) { - return state; + return state; } - return { + ok = true; + return { splitPaneChatrooms: [...currentSplitPanes, targetRoom.id], }; }); + return ok; },
2352-2358: Normalize in removeFromSplitPane for symmetryMinor: normalize the incoming ID here too for consistency with addToSplitPane.
Apply this diff:
- removeFromSplitPane: (chatroomId) => { - set((state) => ({ - splitPaneChatrooms: (state.splitPaneChatrooms || []).filter( - (id) => !safeChatroomIdMatch(id, chatroomId, 'removeFromSplitPane'), - ), - })); - }, + removeFromSplitPane: (chatroomId) => { + const norm = normalizeChatroomIdForSplitPane(chatroomId); + set((state) => ({ + splitPaneChatrooms: (state.splitPaneChatrooms || []).filter( + (id) => !safeChatroomIdMatch(id, norm, 'removeFromSplitPane'), + ), + })); + return norm != null; + },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
docs/Architecture-Overview.md(1 hunks)src/renderer/src/assets/styles/components/Navbar.scss(4 hunks)src/renderer/src/assets/styles/dialogs/mentions.scss(2 hunks)src/renderer/src/components/Dialogs/Mentions.jsx(3 hunks)src/renderer/src/components/Navbar.jsx(5 hunks)src/renderer/src/pages/ChatPage.jsx(3 hunks)src/renderer/src/providers/ChatProvider.jsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/renderer/src/assets/styles/components/Navbar.scss
🧰 Additional context used
🧬 Code graph analysis (2)
src/renderer/src/pages/ChatPage.jsx (4)
src/renderer/src/components/Dialogs/Mentions.jsx (2)
useChatStore(18-43)Mentions(16-184)src/renderer/src/providers/ChatProvider.jsx (4)
useChatStore(423-3961)MAX_SPLIT_PANE_COUNT(404-404)MAX_SPLIT_PANE_COUNT(404-404)settings(3970-3970)src/renderer/src/components/Chat/index.jsx (1)
Chat(14-100)src/renderer/src/components/SplitPaneChat.jsx (1)
SplitPaneChat(6-42)
src/renderer/src/components/Navbar.jsx (3)
src/renderer/src/providers/ChatProvider.jsx (4)
useChatStore(423-3961)MAX_SPLIT_PANE_COUNT(404-404)MAX_SPLIT_PANE_COUNT(404-404)settings(3970-3970)src/renderer/src/components/Shared/Tooltip.jsx (4)
TooltipProvider(7-7)Tooltip(9-9)TooltipTrigger(11-11)TooltipContent(13-17)src/renderer/src/components/Navbar/ChatroomTab.jsx (1)
ChatroomTab(15-190)
⏰ 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). (1)
- GitHub Check: test-builds (windows-latest)
🔇 Additional comments (5)
src/renderer/src/assets/styles/dialogs/mentions.scss (1)
33-35: Header actions alignment looks good.Flex-end + center + gap improves layout consistency.
src/renderer/src/components/Dialogs/Mentions.jsx (1)
81-91: Close button wiring looks correct.Aria label and type="button" are set.
docs/Architecture-Overview.md (1)
31-58: Document split‑pane constraints (limit and min sizes).+#### Split Pane Constraints +KickTalk caps concurrent split panes to `MAX_SPLIT_PANE_COUNT` (currently 3) to satisfy +react-resizable-panels’ minimum-size requirements. The main panel uses `minSize=25` and +each split panel `minSize=20`, keeping the sum ≤ 100% to avoid MinSizesNotSatisfiable errors.src/renderer/src/providers/ChatProvider.jsx (2)
400-401: Ephemeral split-pane state: confirm persistence choicesplitPaneChatrooms is in-memory only (not saved to localStorage). If intentional, all good; if not, persist alongside chatrooms.
2360-2363: LGTMclearSplitPane is simple and correct.
…drop - Replace @hello-pangea/dnd with @atlaskit/pragmatic-drag-and-drop for better wrapping support - Fix drag-and-drop issue where dragging to wrapped rows would drop to first row - Add enhanced auto-scroll with 80px trigger zones and configurable speed - Implement horizontal mouse wheel scrolling for chatroom navigation - Improve layout flow: mentions tab, add button, and split zone now follow wrapped chatrooms - Add accessibility improvements with proper ARIA attributes for tab interface - Enhance event collection logic for better telemetry coverage Breaking change: Drag and drop now uses native HTML5 API instead of transform-based positioning
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (13)
package.json (1)
127-128: Deduplicate React in devDependencies.You already depend on react/react-dom in dependencies. Having them again in devDependencies can cause duplicate installs/conflicts. Remove from devDependencies.
Apply this diff:
"devDependencies": { - "react": "^18.3.1", - "react-dom": "^18.3.1",scripts/event-collection/event-collector.js (4)
242-266: Reply sampling never triggers (no '(reply)' classification).
shouldSampleEvent()checks for(reply)butgetDetailedEventType()never emits it, solastReplyMessageTimeis unused.Proposed fix: detect replies via metadata and label accordingly.
getDetailedEventType(event, data) { const baseEventType = event; // Enhanced classification for ChatMessageEvent if (baseEventType === 'App\\Events\\ChatMessageEvent' && data) { if (data.type) { - if (data.type === 'celebration' && data.metadata?.celebration?.type) { + if (data.type === 'celebration' && data.metadata?.celebration?.type) { const celebrationType = data.metadata.celebration.type; return `App\\Events\\ChatMessageEvent (${data.type} - ${celebrationType})`; } - if (data.type === 'message') { - return `App\\Events\\ChatMessageEvent (regular)`; - } + if (data.type === 'message') { + const isReply = + Boolean( + data.metadata?.reply_to || + data.metadata?.replied_to || + data.metadata?.reply || + data.metadata?.parent_message_id + ); + return `App\\Events\\ChatMessageEvent (${isReply ? 'reply' : 'regular'})`; + } return `App\\Events\\ChatMessageEvent (${data.type})`; } }
330-350: Prevent reconnect storms.Heartbeat reconnects if socket not OPEN, and
closehandler also reconnects after 5s. This can create overlapping connects.Add a simple reconnect guard.
class KickEventCollector { constructor() { ... this.heartbeatInterval = null; + this.reconnectTimer = null; + this.connecting = false; } ... connect() { + if (this.connecting) return; + this.connecting = true; console.log('Connecting to Kick WebSocket...'); this.ws = new WebSocket( "wss://ws-us2.pusher.com/app/32cbd69e4b950bf97679?protocol=7&client=js&version=8.4.0-rc2&flash=false" ); this.ws.on('open', () => { console.log('✓ Connected to Kick WebSocket'); this.startHeartbeat(); + this.connecting = false; }); ... this.ws.on('close', () => { console.log('WebSocket connection closed'); this.stopHeartbeat(); - // Attempt to reconnect after 5 seconds - setTimeout(() => { + if (this.reconnectTimer) clearTimeout(this.reconnectTimer); + this.reconnectTimer = setTimeout(() => { console.log('Attempting to reconnect...'); - this.connect(); - }, 5000); + this.connecting = false; + this.connect(); + }, 5000); }); }Also gate the heartbeat check:
- if (!this.ws || this.ws.readyState !== WebSocket.OPEN) { + if (!this.ws || this.ws.readyState !== WebSocket.OPEN) { + if (this.connecting) return; console.log(`⚠️ WebSocket in bad state (${this.ws?.readyState}), reconnecting...`); this.connect(); }
275-291: Cap raw payload size in logs.Very large
rawDatacan bloat.jsonl. Consider truncating or gating by size.Example:
- if (message.data) { + if (message.data) { rawData = message.data; try { data = JSON.parse(rawData); } catch (error) { ... } } + // Optional: cap rawData to avoid huge files + const MAX_RAW_LEN = 50_000; + if (rawData && rawData.length > MAX_RAW_LEN) { + rawData = rawData.slice(0, MAX_RAW_LEN) + '…(truncated)'; + }
392-399: Clear the 60s status interval on shutdown.You log final stats on SIGINT, but the interval keeps running until process.exit. Clean it for hygiene.
- // Status update every 60 seconds - setInterval(() => { + // Status update every 60 seconds + this.statusInterval = setInterval(() => { ... - }, 60000); + }, 60000);And inside SIGINT handler:
if (this.ws) { this.ws.close(); } +if (this.statusInterval) { + clearInterval(this.statusInterval); +}src/renderer/src/components/Navbar/ChatroomTab.jsx (2)
72-101: Ensure dragging state resets on cancel.Relying on
onDropalone can leaveisDraggingtrue if the drag is cancelled. AddonDragEndto reset.draggable({ element, getInitialData: getDraggableData, onDragStart: () => setIsDragging(true), - onDrop: () => setIsDragging(false), + onDrop: () => setIsDragging(false), + onDragEnd: () => setIsDragging(false), }),
162-168: A11y: unread status element should have discernible text when announced.
role="status"with an empty element gives no info to AT. Provide a short label (e.g., “unread messages”) and keep it visually hidden.- <span + <span id={`chatroom-unread-${chatroom.id}`} className={clsx("unreadCountIndicator", unreadCount > 0 && "hasUnread")} role="status" aria-live="polite" aria-hidden={unreadCount === 0} -/> +> + {unreadCount > 0 ? 'unread messages' : ''} + </span>Alternatively, expose the count via
aria-labelon the tab.src/renderer/src/assets/styles/components/Navbar.scss (2)
111-146: Avoid duplicated split-zone styles; unify via a shared rule.Same class names are defined twice (wrapped vs. global). Centralize common rules and override only deltas to reduce drift.
Minimal example:
+// shared split zone +.splitPaneDropZoneContainer { flex-shrink: 0; } +.splitPaneDropZone { + border: 2px solid transparent; + transition: border .2s, background .2s, color .2s; + cursor: default; +} +.splitPaneDropZone.active { border: 2px dashed rgba(255,255,255,.4); background: rgba(255,255,255,.2); color: rgba(255,255,255,.8); } // keep only size/position differences in the wrapped scope .wrapChatroomList .chatroomsList { .splitPaneDropZoneContainer { height: 40px; } .splitPaneDropZone { height: 40px; width: 40px; } } // global variant sizes -.splitPaneDropZoneContainer { height: 32px; width: 80px; } -.splitPaneDropZone { height: 32px; width: 80px; padding: 1px 16px; } +.splitPaneDropZoneContainer { height: 32px; width: 80px; } +.splitPaneDropZone { height: 32px; width: 80px; padding: 1px 16px; }Also applies to: 688-736
27-41: Scroll semantics: prefer auto over scroll to avoid always-on scrollbars.
overflow-x: autoavoids showing a disabled scrollbar when content fits.- overflow-x: scroll; + overflow-x: auto;src/renderer/src/components/Navbar.jsx (4)
103-133: Reorder logic: guard against stale indices after interim store updates.Between drag start and drop,
orderedChatroomsmay change (e.g., a tab closed). Add safety checks and id-based reorder.- const sourceIndex = source.data.index; - const destinationIndex = destination.data.index; - if (sourceIndex !== destinationIndex) { - const reordered = Array.from(orderedChatrooms); - const [removed] = reordered.splice(sourceIndex, 1); - reordered.splice(destinationIndex, 0, removed); - reorderChatrooms(reordered); - } + const sourceId = source.data.chatroomId; + const destIndex = destination.data.index; + const current = Array.from(orderedChatrooms); + const from = current.findIndex(c => c.id === sourceId); + const to = Math.max(0, Math.min(destIndex, current.length - 1)); + if (from !== -1 && to !== -1 && from !== to) { + const [removed] = current.splice(from, 1); + current.splice(to, 0, removed); + reorderChatrooms(current); + }
142-173: Wheel-to-horizontal scroll: add Firefox support and avoid smooth on rapid events.Firefox doesn’t support
wheelDeltathe same; you’re OK usingdeltaY, but smooth scrolling each tick can queue animations. Switch to instant behavior for responsiveness.- navbarContainer.scrollBy({ - left: scrollAmount, - behavior: 'smooth' - }); + navbarContainer.scrollLeft += scrollAmount;
248-274: Split-zone drop: accept by id rather than capacity snapshot.
canAcceptMoreSplitPanesis captured at effect setup; if capacity changes during a drag, handlers may be stale. Use latest via ref.+ const canAcceptMoreRef = useRef(canAcceptMoreSplitPanes); + useEffect(() => { canAcceptMoreRef.current = canAcceptMoreSplitPanes; }, [canAcceptMoreSplitPanes]); useEffect(() => { const element = splitZoneRef.current; if (!element) return; return dropTargetForElements({ element, getData: () => ({ type: 'split-zone' }), canDrop: ({ source }) => { - return source.data.type === 'chatroom-tab' && canAcceptMoreSplitPanes; + return source.data.type === 'chatroom-tab' && canAcceptMoreRef.current; }, onDragEnter: () => { - if (canAcceptMoreSplitPanes) { + if (canAcceptMoreRef.current) { setIsDraggingOverSplit(true); } }, onDragLeave: () => { setIsDraggingOverSplit(false); }, onDrop: ({ source }) => { setIsDraggingOverSplit(false); - if (source.data.type === 'chatroom-tab' && canAcceptMoreSplitPanes) { + if (source.data.type === 'chatroom-tab' && canAcceptMoreRef.current) { addToSplitPane(source.data.chatroomId); } }, }); -}, [canAcceptMoreSplitPanes, addToSplitPane]); +}, [addToSplitPane]);
276-299: Auto-scroll thresholds: make them responsive to container size.Fixed 80px edges feel off on small/large windows. Use a fraction of clientWidth/Height.
- threshold: { top: 80, bottom: 80, left: 80, right: 80 } + threshold: ({ element }) => { + const w = element.clientWidth, h = element.clientHeight; + const edgeX = Math.max(40, Math.round(w * 0.08)); + const edgeY = Math.max(40, Math.round(h * 0.08)); + return { top: edgeY, bottom: edgeY, left: edgeX, right: edgeX }; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
package.json(2 hunks)scripts/event-collection/event-collector.js(6 hunks)src/renderer/src/assets/styles/components/Navbar.scss(3 hunks)src/renderer/src/components/Navbar.jsx(6 hunks)src/renderer/src/components/Navbar/ChatroomTab.jsx(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/renderer/src/components/Navbar/ChatroomTab.jsx (1)
src/renderer/src/components/Shared/ContextMenu.jsx (2)
ContextMenu(9-9)ContextMenuTrigger(11-11)
src/renderer/src/components/Navbar.jsx (4)
src/renderer/src/providers/ChatProvider.jsx (5)
useChatStore(423-3961)MAX_SPLIT_PANE_COUNT(404-404)MAX_SPLIT_PANE_COUNT(404-404)settings(3970-3970)span(56-56)src/renderer/src/pages/ChatPage.jsx (2)
splitPaneChatrooms(89-89)span(53-53)src/renderer/src/components/Shared/Tooltip.jsx (4)
Tooltip(9-9)TooltipTrigger(11-11)TooltipContent(13-17)TooltipProvider(7-7)src/renderer/src/components/Navbar/ChatroomTab.jsx (1)
ChatroomTab(16-230)
⏰ 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). (2)
- GitHub Check: test-builds (macos-latest)
- GitHub Check: test-builds (windows-latest)
🔇 Additional comments (1)
package.json (1)
64-65: New DnD and split-pane deps look correct for the feature.No issues spotted with the versions for pragmatic-dnd and react-resizable-panels.
Also applies to: 97-97
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (14)
package.json (1)
63-103: Reproducible builds for Electron: prefer exact pins for newly introduced UI libs.New UI libs directly affect renderer UX. Exact pins (or overrides) reduce supply‑chain drift; keep caret for mature infra deps if preferred.
If you’d rather keep carets, add only an overrides block:
+ "overrides": { + "react-resizable-panels": "3.0.6", + "@atlaskit/pragmatic-drag-and-drop": "1.7.7", + "@atlaskit/pragmatic-drag-and-drop-auto-scroll": "2.1.2" + },src/renderer/src/components/Navbar.jsx (3)
151-179: Don’t hijack vertical wheel scroll when wrapping is enabled.In wrap mode, converting vertical wheel to horizontal prevents users from scrolling vertically if content wraps. Gate the handler by
wrapChatroomsList.-useEffect(() => { - const handleWheel = (e) => { +useEffect(() => { + if (wrapChatroomsList) return; + const handleWheel = (e) => { const navbarContainer = navbarContainerRef.current; if (!navbarContainer) return; - // Only handle vertical wheel events (convert to horizontal scroll) - if (Math.abs(e.deltaY) > Math.abs(e.deltaX)) { + // Only handle vertical wheel events (convert to horizontal scroll) + if (Math.abs(e.deltaY) > Math.abs(e.deltaX)) { e.preventDefault(); // Scroll horizontally based on vertical wheel movement const scrollAmount = e.deltaY > 0 ? 60 : -60; navbarContainer.scrollLeft += scrollAmount; } }; - const navbarContainer = navbarContainerRef.current; + const navbarContainer = navbarContainerRef.current; if (navbarContainer) { navbarContainer.addEventListener("wheel", handleWheel, { passive: false }); } return () => { if (navbarContainer) { navbarContainer.removeEventListener("wheel", handleWheel); } }; -}, []); +}, [wrapChatroomsList]);
252-281: Split-zone drop target: behavior OK; add an accessible state.Expose disabled state to AT when capacity is full.
- onDrop: ({ source }) => { + onDrop: ({ source }) => { setIsDraggingOverSplit(false); if (source.data.type === 'chatroom-tab' && canAcceptMoreRef.current) { addToSplitPane(source.data.chatroomId); } },And in the renderer below, add
aria-disabled:- <div + <div ref={splitZoneRef} className={clsx( "splitPaneDropZone", !canAcceptMoreSplitPanes && "disabled", isDraggingOverSplit && canAcceptMoreSplitPanes && "active", )} + aria-disabled={!canAcceptMoreSplitPanes} + aria-label="Split pane drop zone" >
313-336: Minor UX: provide non-drag path to split.Consider adding “Open in split pane” to the chatroom tab context menu for keyboard-only users (see ChatroomTab.jsx menu block). Keeps feature reachable without drag.
src/renderer/src/assets/styles/components/Navbar.scss (3)
1-10: Contain horizontal scroll chaining.Add
overscroll-behavior-x: contain;to prevent the page/body from scrolling while wheel-scrolling the navbar horizontally..navbarContainer { min-height: 80px; height: auto; width: 100%; - overflow-x: auto; + overflow-x: auto; + overscroll-behavior-x: contain; background-color: var(--navbar-bg);
673-721: Improve split-zone affordance and motion sensitivity.
- Use a more descriptive cursor when droppable.
- Respect reduced-motion preferences for the icon scale effect.
.splitPaneDropZone { ... - cursor: default; + cursor: default; ... &.active { border: 2px dashed rgba(255, 255, 255, 0.4); background: rgba(255, 255, 255, 0.2); color: rgba(255, 255, 255, 0.8); + cursor: copy; } ... - &.active svg { - transform: scale(1.1); - } + @media (prefers-reduced-motion: no-preference) { + &.active svg { + transform: scale(1.1); + transition: transform 120ms ease-out; + } + } }
683-701: Text contrast nit.
color: rgba(255,255,255,0.3)for “Split” may be low on some themes; consider 0.5 for better readability.- color: rgba(255, 255, 255, 0.3); + color: rgba(255, 255, 255, 0.5);scripts/event-collection/event-collector.js (7)
118-125: Guard looks good; add open‑state short‑circuitYou already prevent concurrent connects via
this.connecting. Also skip if a socket is already open to avoid redundant work during rare races.connect() { - if (this.connecting) { + if (this.connecting) { return; } + if (this.ws && this.ws.readyState === WebSocket.OPEN) { + return; + } this.connecting = true;Also applies to: 131-136
374-379: Unify reconnect policy in heartbeatHeartbeat directly calls
connect(). Prefer scheduling a reconnect via the same path as the close handler to keep behavior consistent and avoid dueling strategies.- console.log(`⚠️ WebSocket in bad state (${this.ws?.readyState}), reconnecting...`); - this.connect(); + console.log(`⚠️ WebSocket in bad state (${this.ws?.readyState}), scheduling reconnect...`); + if (!this.reconnectTimer) { + this.reconnectTimer = setTimeout(() => { + if (this.connecting || (this.ws && this.ws.readyState === WebSocket.OPEN)) return; + this.connect(); + }, 0); + }If you add a
scheduleReconnect()helper, call it here instead.
230-237: Capturing all non‑pusher events may explode log volumeThis will log any third‑party or noisy internal events that don’t start with
pusher. Consider a lightweight denylist or rate‑limit for high‑volume types to protect disk and I/O.
268-292: Sampling uses global clocks; consider per‑channel throttlingWith a single
lastRegularMessageTime/lastReplyMessageTime, one busy channel can starve others. Track last‑sample times per channel (e.g., Map keyed by channel + type) to distribute samples fairly.If you want, I can draft a small patch introducing
lastSampleByChannel = Map<string, {regular:number, reply:number}>.
298-313: JSON parse warnings could be noisy; add rate‑limitMalformed payloads can flood the console. Consider a simple counter and log at most N warnings per minute.
355-355: Avoid blocking I/O on the event loop
fs.appendFileSyncblocks on every event. For high‑throughput channels, switch to a singlefs.createWriteStreamwith backpressure handling.Example (outside this hunk):
// at construction this.logStream = fs.createWriteStream(this.logFile, { flags: 'a' }); // in logEvent this.logStream.write(JSON.stringify(logEntry) + '\n'); // on shutdown this.logStream.end();
420-427: Cleanup on SIGINT is thoroughNice touch clearing timers (including the new
statusInterval) and closing the socket.Optionally call
this.stopHeartbeat()here to avoid relying on the impendingclosecallback ordering.Also applies to: 432-436
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
package.json(2 hunks)scripts/event-collection/event-collector.js(11 hunks)src/renderer/src/assets/styles/components/Navbar.scss(4 hunks)src/renderer/src/components/Navbar.jsx(6 hunks)src/renderer/src/components/Navbar/ChatroomTab.jsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/renderer/src/components/Navbar/ChatroomTab.jsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/renderer/src/components/Navbar.jsx (4)
src/renderer/src/providers/ChatProvider.jsx (5)
useChatStore(423-3961)MAX_SPLIT_PANE_COUNT(404-404)MAX_SPLIT_PANE_COUNT(404-404)settings(3970-3970)span(56-56)src/renderer/src/pages/ChatPage.jsx (2)
splitPaneChatrooms(89-89)span(53-53)src/renderer/src/components/Shared/Tooltip.jsx (4)
Tooltip(9-9)TooltipTrigger(11-11)TooltipContent(13-17)TooltipProvider(7-7)src/renderer/src/components/Navbar/ChatroomTab.jsx (1)
ChatroomTab(16-233)
⏰ 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). (2)
- GitHub Check: test-builds (windows-latest)
- GitHub Check: test-builds (macos-latest)
🔇 Additional comments (7)
package.json (2)
64-66: Pin PDD versions and verify cross‑package compatibility (core v1 + auto‑scroll v2).Sandbox returned no usable output for the npm checks — run these locally to confirm peerDependencies and installed versions, then pin exact versions in package.json (file: package.json, lines ~64–66).
- "@atlaskit/pragmatic-drag-and-drop": "^1.7.7", - "@atlaskit/pragmatic-drag-and-drop-auto-scroll": "^2.1.2", + "@atlaskit/pragmatic-drag-and-drop": "1.7.7", + "@atlaskit/pragmatic-drag-and-drop-auto-scroll": "2.1.2",Optional root overrides:
+ "overrides": { + "@atlaskit/pragmatic-drag-and-drop": "1.7.7", + "@atlaskit/pragmatic-drag-and-drop-auto-scroll": "2.1.2" + },Run to confirm peer deps and installed versions:
npm view @atlaskit/pragmatic-drag-and-drop@1.7.7 peerDependencies --json npm view @atlaskit/pragmatic-drag-and-drop-auto-scroll@2.1.2 peerDependencies --json npm ls @atlaskit/pragmatic-drag-and-drop @atlaskit/pragmatic-drag-and-drop-auto-scroll react react-dom --depth=0
97-97: Lock react-resizable-panels to 3.0.6 and verify project React version.react-resizable-panels@3.0.6 peerDependencies: react '^16.14.0 || ^17.0.0 || ^18.0.0 || ^19.0.0' (React 18.3.1 is compatible). The verification run's
npm lsreturned an empty tree — unable to confirm the repo's installed react/react-dom versions.Apply this change in package.json:
- "react-resizable-panels": "^3.0.6", + "react-resizable-panels": "3.0.6",Verify locally by checking package.json or running:
npm ls react react-dom react-resizable-panels --depth=0
Ensure the project's React/React‑DOM versions fall within the peer range (e.g., 18.3.1).src/renderer/src/components/Navbar.jsx (3)
42-49: Good use of a ref to avoid stale closures in DnD callbacks.Updating
canAcceptMoreRefviauseEffectis the right pattern for pragmatic-dnd handlers.
108-142: DnD monitor logic looks solid.Reorder-on-drop is bounded and guarded; early exits prevent misfires. No issues.
282-312: Auto-scroll config LGTM.Reasonable thresholds and speeds; cleanup handled.
scripts/event-collection/event-collector.js (2)
250-257: Reply vs regular classification LGTMThe reply detection across multiple metadata keys is pragmatic and resilient.
29-31: Per‑reply sampling state LGTMSeparate timers for replies vs regular messages make sense for observability.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
scripts/event-collection/event-collector.js (1)
334-340: Large payload risk remains with sanitized message inrawfieldWhile you're truncating
rawData, thesanitizedMessagestored inlogEntry.rawstill contains the full truncated data reference. However, since you're creating a shallow copy with the truncated data, this should be acceptable for size constraints.
🧹 Nitpick comments (6)
src/renderer/src/assets/styles/components/Navbar.scss (4)
5-6: Scroll containment added; consider mobile inertia.Nice guard against horizontal scroll chaining. If you target mobile Safari, add smooth inertial scrolling.
.navbarContainer { min-height: 80px; height: auto; width: 100%; overflow-x: auto; overscroll-behavior-x: contain; + -webkit-overflow-scrolling: touch; /* iOS Safari only; harmless elsewhere */
36-41: Deduplicate shared.chatroomsContainerrules.Both blocks set identical base styles; keep common props in the first block and only override deltas in wrap mode to reduce override churn.
- .chatroomsContainer { - display: flex; - flex-wrap: wrap; - gap: 8px; - align-items: center; - flex: 1 1 auto; - min-width: 0; - &.chatroomsContainerWrapMode { - align-content: flex-start; - } - } + .chatroomsContainer { + /* inherits display:flex; align-items:center from the base block */ + flex-wrap: wrap; + gap: 8px; + flex: 1 1 auto; + min-width: 0; + &.chatroomsContainerWrapMode { align-content: flex-start; } + }Also applies to: 56-67
112-131: Drop zone in wrap mode lacks keyboard focus affordance.Add a visible
:focus-visiblestyle and ensure the hit area meets 44px touch target where feasible..splitPaneDropZone { position: static; height: 40px; width: 40px; padding: 0; justify-content: center; + outline: none; + + &:focus-visible { + border: 2px solid var(--focus-ring, #9fd1ff); + background: rgba(255,255,255,0.18); + }
674-726: Global drop zone: improve a11y and disabled behavior; consider larger target.
- Add
:focus-visiblefor keyboard users.- Make
.disablednon-interactive (pointer-events: none).- If layout allows, bump height from 32px to 40px for consistency with other controls.
.splitPaneDropZoneContainer { - height: 32px; + height: 32px; /* consider 40px if space permits */ width: 80px; padding: 0; flex-shrink: 0; } .splitPaneDropZone { position: absolute; top: 0; left: 0; - height: 32px; + height: 32px; /* consider 40px */ width: 80px; padding: 1px 16px; font-size: 15px; display: flex; align-items: center; gap: 8px; border-radius: 4px; background: transparent; color: rgba(255, 255, 255, 0.5); border: 2px solid transparent; cursor: default; transition: border 0.2s ease-in-out, background 0.2s ease-in-out, color 0.2s ease-in-out; user-select: none; + outline: none; + + &:focus-visible { + border: 2px solid var(--focus-ring, #9fd1ff); + background: rgba(255,255,255,0.18); + color: rgba(255,255,255,0.9); + } &.active { border: 2px dashed rgba(255, 255, 255, 0.4); background: rgba(255, 255, 255, 0.2); color: rgba(255, 255, 255, 0.8); cursor: copy; } &.disabled { cursor: not-allowed; opacity: 0.4; + pointer-events: none; }QA asks:
- With keyboard only, tab to the drop zone: is there a visible focus ring?
- When
.disabled, confirm it doesn’t accept drops or clicks.- On small window widths, verify the 32→40px change (if applied) doesn’t force unwanted wrapping.
src/renderer/src/components/Navbar/ChatroomTab.jsx (1)
165-173: Consider improving unread indicator text for screen readersThe unread indicator shows "unread messages" but doesn't include the count. Screen reader users would benefit from knowing the actual count.
- {unreadCount > 0 ? "unread messages" : ""} + {unreadCount > 0 ? `${unreadCount} unread messages` : ""}src/renderer/src/components/Navbar.jsx (1)
152-176: Consider debouncing the wheel scroll handlerThe horizontal scroll implementation via wheel events could benefit from debouncing to improve performance on rapid scroll events.
Consider adding a simple debounce or using requestAnimationFrame:
useEffect(() => { const navbarContainer = navbarContainerRef.current; if (!navbarContainer || wrapChatroomsList) { return undefined; } + let scrollRAF = null; const handleWheel = (e) => { // Only handle vertical wheel events (convert to horizontal scroll) if (Math.abs(e.deltaY) > Math.abs(e.deltaX)) { e.preventDefault(); + if (scrollRAF) { + cancelAnimationFrame(scrollRAF); + } + scrollRAF = requestAnimationFrame(() => { // Scroll horizontally based on vertical wheel movement // Increase scroll amount for more responsive scrolling const scrollAmount = e.deltaY > 0 ? 60 : -60; navbarContainer.scrollLeft += scrollAmount; + }); } }; navbarContainer.addEventListener("wheel", handleWheel, { passive: false }); return () => { + if (scrollRAF) { + cancelAnimationFrame(scrollRAF); + } navbarContainer.removeEventListener("wheel", handleWheel); }; }, [wrapChatroomsList]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
package.json(3 hunks)scripts/event-collection/event-collector.js(12 hunks)src/renderer/src/assets/styles/components/Navbar.scss(4 hunks)src/renderer/src/components/Navbar.jsx(6 hunks)src/renderer/src/components/Navbar/ChatroomTab.jsx(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🧬 Code graph analysis (2)
src/renderer/src/components/Navbar/ChatroomTab.jsx (1)
src/renderer/src/components/Shared/ContextMenu.jsx (3)
ContextMenu(9-9)ContextMenuTrigger(11-11)ContextMenuItem(41-43)
src/renderer/src/components/Navbar.jsx (4)
src/renderer/src/providers/ChatProvider.jsx (5)
useChatStore(423-3961)MAX_SPLIT_PANE_COUNT(404-404)MAX_SPLIT_PANE_COUNT(404-404)settings(3970-3970)span(56-56)src/renderer/src/pages/ChatPage.jsx (2)
splitPaneChatrooms(89-89)span(53-53)src/renderer/src/components/Shared/Tooltip.jsx (4)
Tooltip(9-9)TooltipTrigger(11-11)TooltipContent(13-17)TooltipProvider(7-7)src/renderer/src/components/Navbar/ChatroomTab.jsx (1)
ChatroomTab(16-241)
🪛 Biome (2.1.2)
src/renderer/src/components/Navbar.jsx
[error] 176-176: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
🔇 Additional comments (11)
src/renderer/src/assets/styles/components/Navbar.scss (2)
69-76: Trailing controls layout looks solid.Fixed-size controls won’t collapse during wrap; separator sizing matches chip height.
Also applies to: 78-82
84-92: Specificity override is correct; validate no overlap with absolute variant.This scoped reset to
position: staticshould beat the global absolute style. Verify in compact+wrap combos that the absolute gradient container (Line 374 block) doesn’t overlap these controls.src/renderer/src/components/Navbar/ChatroomTab.jsx (2)
74-104: Ensure cleanup happens for all drag/drop handlersThe drag-and-drop setup properly uses
combine()to merge cleanup functions, but verify that both the draggable and dropTarget cleanup functions are executed when dependencies change or component unmounts.
222-227: LGTM! Split pane menu integration looks goodThe "Open in Split Pane" context menu item is properly integrated with appropriate disabled state based on capacity. The implementation correctly uses the
onOpenInSplitPanecallback andcanOpenInSplitPaneprop.scripts/event-collection/event-collector.js (3)
53-72: LGTM! Reconnection logic properly guards against concurrent attemptsThe
scheduleReconnectmethod correctly checks for existing connections and handles the timer cleanup well. This addresses the previous review comment about concurrent reconnects.
139-146: Connection guard properly prevents concurrent attemptsThe connection method now correctly guards against concurrent connection attempts by checking both the
connectingflag and WebSocket state.
395-402: Good improvement to heartbeat reconnection logicThe heartbeat now properly checks the
connectingflag before attempting reconnection and uses the centralizedscheduleReconnectmethod.src/renderer/src/components/Navbar.jsx (4)
109-142: LGTM! Drag and drop monitoring implementationThe migration to Atlassian's pragmatic drag-and-drop is well implemented. The monitor properly handles both reordering within the chatroom list and the split zone interaction.
252-277: Split zone drop target implementation looks solidThe split zone properly checks capacity before accepting drops and maintains the drag-over state for visual feedback. Good use of the ref-based capacity check to avoid stale closures.
279-308: Auto-scroll configuration is well-tunedThe auto-scroll setup for the navbar container has appropriate speed settings and threshold calculations that scale based on the container size.
315-340: Split pane drop zone UI provides good user feedbackThe tooltip content adapts based on capacity, providing clear feedback about the limit when reached. The visual states (disabled/active) are properly managed.
- Add `-webkit-overflow-scrolling: touch` for smooth scrolling on iOS/macOS. - Implement RAF-based horizontal scrolling for wheel events to prevent jank. - Introduce `wrapChatroomsList` setting integration for flexible layout. - Add `align-items: center` to chatrooms container for better alignment. - Enhance accessibility with `focus-visible` styles (borders, backgrounds) and `pointer-events: none` for disabled states.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
src/renderer/src/components/Navbar/ChatroomTab.jsx (3)
79-83: Consider including the display name in drag data for better UX.The drag data currently only includes the chatroom ID and index. Including the display name would enable richer UI feedback during drag operations (e.g., showing the chatroom name in drag preview or drop zones).
const getDraggableData = () => ({ type: 'chatroom-tab', chatroomId: chatroom.id, + displayName: chatroom.displayName || chatroom.username, index, });
165-173: Fix accessibility issue with unread count indicator.The unread count indicator has conflicting accessibility attributes. When
unreadCount === 0, the element shows an empty string but hasaria-hidden={true}andaria-live="polite"simultaneously, which can confuse screen readers.<span id={`chatroom-unread-${chatroom.id}`} className={clsx("unreadCountIndicator", unreadCount > 0 && "hasUnread")} - role="status" - aria-live="polite" - aria-hidden={unreadCount === 0} + role={unreadCount > 0 ? "status" : undefined} + aria-live={unreadCount > 0 ? "polite" : undefined} + aria-hidden={unreadCount === 0 ? "true" : undefined} > {unreadCount > 0 ? `${unreadCount} unread messages` : ""} </span>
125-129: Consider preventing default for middle-click to avoid platform-specific behaviors.Middle-click handling in
onMouseDownmight conflict with browser defaults (e.g., opening in new tab on some platforms). Consider preventing the default behavior.onMouseDown={async (e) => { if (e.button === 1) { + e.preventDefault(); await onRemoveChatroom(chatroom.id); } }}src/renderer/src/assets/styles/components/Navbar.scss (4)
37-42: Prevent flex child overflow with min-width: 0.In the non-wrap layout, long titles can overflow/fail to ellipsize inside flex containers without min-width: 0. You added it for wrap mode; mirror it here.
Apply this diff:
.chatroomsContainer { display: flex; gap: 12px; height: auto; align-items: center; + min-width: 0; }
77-81: Separator specificity: minor consistency.This block sets flex/height/margin but relies on the global
.chatroomsSeparatorfor width/background. Consider adding width here to avoid cascade surprises..chatroomsSeparator { flex: 0 0 1px; height: 28px; margin: 0; + width: 1px; }
678-739: DRY up duplicated drop-zone styles and add touch-action.These global definitions duplicate the wrapped versions. Extract shared rules into a placeholder/mixin to keep sizes as modifiers. Also add touch-action: none here.
Apply this minimal safety diff:
.splitPaneDropZone { position: absolute; top: 0; left: 0; height: 32px; width: 80px; padding: 1px 16px; font-size: 15px; display: flex; align-items: center; gap: 8px; border-radius: 4px; background: transparent; color: rgba(255, 255, 255, 0.5); border: 2px solid transparent; cursor: default; transition: border 0.2s ease-in-out, background 0.2s ease-in-out, color 0.2s ease-in-out; user-select: none; outline: none; + touch-action: none;Optional SCSS refactor sketch:
%split-pane-drop-zone-base { display: flex; align-items: center; gap: 8px; border-radius: 4px; background: transparent; color: rgba(255,255,255,.5); border: 2px solid transparent; transition: border .2s, background .2s, color .2s; user-select: none; outline: none; touch-action: none; &:focus-visible { border: 2px solid var(--focus-ring, #9fd1ff); background: rgba(255,255,255,.18); color: rgba(255,255,255,.9); } &.active { border: 2px dashed rgba(255,255,255,.4); background: rgba(255,255,255,.2); color: rgba(255,255,255,.8); cursor: copy; } &.disabled { cursor: not-allowed; opacity: .4; pointer-events: none; } svg { flex-shrink: 0; pointer-events: none; } } .wrapChatroomList .splitPaneDropZone { @extend %split-pane-drop-zone-base; position: static; height: 40px; width: 40px; padding: 0; justify-content: center; } .splitPaneDropZone { @extend %split-pane-drop-zone-base; position: absolute; top: 0; left: 0; height: 32px; width: 80px; padding: 1px 16px; font-size: 15px; }
111-136: Drop zone accessibility — add touch-action and keyboard focus/role
- CSS: add touch-action to the drop zone (src/renderer/src/assets/styles/components/Navbar.scss).
.splitPaneDropZone { position: static; height: 40px; width: 40px; padding: 0; justify-content: center; outline: none; + touch-action: none;
- Markup: src/renderer/src/components/Navbar.jsx (renderSplitPaneDropZone — ~lines 326–336) already has aria-disabled but lacks keyboard semantics. Add tabIndex={0} and role="button" to the element with className "splitPaneDropZone", or replace the div with a semantic and preserve aria-disabled.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/renderer/src/assets/styles/components/Navbar.scss(4 hunks)src/renderer/src/components/Chat/StreamerInfo.jsx(4 hunks)src/renderer/src/components/Navbar.jsx(6 hunks)src/renderer/src/components/Navbar/ChatroomTab.jsx(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/renderer/src/components/Chat/StreamerInfo.jsx
- src/renderer/src/components/Navbar.jsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/renderer/src/components/Navbar/ChatroomTab.jsx (1)
src/renderer/src/components/Shared/ContextMenu.jsx (3)
ContextMenu(9-9)ContextMenuTrigger(11-11)ContextMenuItem(41-43)
🔇 Additional comments (7)
src/renderer/src/components/Navbar/ChatroomTab.jsx (2)
89-91: Ensure drag state is always cleared on drag end.The drag handlers set
isDraggingto false in bothonDropandonDragEnd. SinceonDropmay not always fire (e.g., invalid drop target), this redundancy is good. However, consider adding error handling to ensure the state is always cleared.The redundant state clearing in both
onDropandonDragEndis a good defensive practice that ensures the UI state is properly reset even if the drop operation fails.
222-227: LGTM! Split pane integration is well implemented.The split pane context menu item is properly integrated with appropriate props (
onOpenInSplitPaneandcanOpenInSplitPane) and correctly handles the disabled state to prevent exceeding the maximum split pane count.src/renderer/src/assets/styles/components/Navbar.scss (5)
5-7: Mobile scrolling behavior: looks good.Good call on horizontal scroll + overscroll containment; the iOS momentum scroll hint is harmless in Chromium/Electron.
50-55: Wrap-mode spacing/readability: LGTM.The wrap gaps and padding work well for multi-row layouts.
57-66: Wrap container flex semantics: LGTM.Flex growth + min-width: 0 and optional wrap-mode alignment are appropriate.
84-91: Add button container override: LGTM.Static positioning + aligning fixes the wrap layout nicely.
68-75: Trailing controls: class defined but not used — verify alignment on wrap
- src/renderer/src/assets/styles/components/Navbar.scss defines .chatroomsTrailingControls (margin-left: auto; flex: 0 0 auto;) which would pin a grouped trailing-controls element to the right when .chatroomsList wraps.
- src/renderer/src/components/Navbar.jsx does not render a wrapper with that class — separator, add button and split-drop-zone are individual siblings (.chatroomsSeparator, .navbarAddChatroomContainer, .splitPaneDropZoneContainer). Either wrap those controls in an element with .chatroomsTrailingControls or ensure the actual elements have equivalent flex rules, then manually test resize/overflow to confirm the controls stay right-aligned on the last row.
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback". |
* Fix cross-platform icon compatibility - Add cross-platform icon handling for Linux/macOS support - Convert Windows .ico to .png for non-Windows platforms - Update tray and thumbar icons to use platform-appropriate format Fixes application crashes on Linux due to unsupported .ico format 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * fix(chat): correct chatters selector for @ mention autocomplete Fixes the data selector for chatters in the chat input component. The selector was looking for chatters in state.chatrooms[].chatters but the data is actually stored in state.chatters[chatroomId]. Resolves #40 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com>
1 participant
Summary
• Add react-resizable-panels for side-by-side chat viewing
• Implement drag-and-drop from navbar tabs to split zone
• Add split pane state management in ChatProvider
• Create SplitPaneChat component for split panel content
• Add close button functionality for removable panels
• Support flexible panel management where any panel can be closed
• Include proper minimum panel sizes for usable interface
• Fix chat input width for split panel layouts
Test plan
Summary by CodeRabbit
New Features
Bug Fixes
Style
Documentation
Chores