feat(desktop): extract sub-components from MessagesApp.tsx (partial #657)#1877
Conversation
|
Warning Review limit reached
Next review available in: 2 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughChangesMessages UI refactor
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
|
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| })); | ||
|
|
||
| // Expose scroll-to-latest via ref | ||
| useEffect(() => { |
There was a problem hiding this comment.
WARNING: Mount-time auto-scroll doesn't sync autoScrollRef / atBottom
This effect scrolls to the bottom on first mount whenever messages.length > 0, but it never sets autoScrollRef.current = true (unlike scrollToLatest and MessageListHandle.scrollToBottom). In the parent, autoScrollRef decides whether incoming messages auto-scroll or increment the "new messages" badge. After this mount scroll the user is visually at the bottom, yet autoScrollRef may still be false, so the next arriving message can incorrectly surface a "X new" badge even though the viewport is already at the latest. Consider setting autoScrollRef.current = true / setAtBottom(true) here, or gating the effect on initial-load intent.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| const messageListRef = useRef<HTMLDivElement>(null); | ||
| const messagesEndRef = useRef<HTMLDivElement>(null); | ||
|
|
||
| useImperativeHandle(ref, () => ({ |
There was a problem hiding this comment.
SUGGESTION: useImperativeHandle is called without a dependency array, so the handle object is re-created on every render.
Add an explicit dependency array since the handle only reads the stable messagesEndRef.
| useImperativeHandle(ref, () => ({ | |
| useImperativeHandle(ref, () => ({ | |
| scrollToBottom: () => { | |
| messagesEndRef.current?.scrollIntoView({ behavior: "smooth" }); | |
| }, | |
| }), [messagesEndRef]); |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: No New Issues Found (incremental) | Recommendation: Merge Overview
Incremental Changes (commit
|
| Severity | Count |
|---|---|
| CRITICAL | 0 |
| WARNING | 0 |
| SUGGESTION | 0 |
Incremental Changes (commit e0fd90f)
The only file changed since the previous review was desktop/src/apps/chat/MessageList.tsx (+3 / -10). All three prior Kilo findings on that file are resolved by this commit:
- Line 195 —
useImperativeHandlenow uses an explicit dependency array ([]), so the handle is no longer re-created every render. (previous SUGGESTION fixed) - Line 318 / 321 — the
message-list-drop-targetclass is restored on the scroll container. (previous SUGGESTION fixed) - Line ~202 — the orphaned mount-time auto-scroll
useEffectwas removed, eliminating theautoScrollRef/atBottomdesync that could surface a spurious "X new" badge. (previous WARNING resolved)
No new issues were introduced by the changed lines.
Out-of-scope (unchanged since previous review)
Six active inline comments on ChannelSidebar.tsx, MessageInput.tsx, MessagesApp.tsx (lines 548, 1130, 88, 1586, 1775, 1950) and one on MessageList.tsx (line 379) belong to files/regions not touched by this incremental commit. They remain valid but are not re-reported here.
Files Reviewed (1 changed)
desktop/src/apps/chat/MessageList.tsx- 3 prior issues resolved, 0 new issues
Previous review (commit b6acf8f)
Status: 3 Issues Found | Recommendation: Address before merge
Overview
| Severity | Count |
|---|---|
| CRITICAL | 0 |
| WARNING | 1 |
| SUGGESTION | 2 |
Issue Details (click to expand)
CRITICAL
| File | Line | Issue |
|---|
WARNING
| File | Line | Issue |
|---|---|---|
desktop/src/apps/chat/MessageList.tsx |
202 | Mount-time auto-scroll does not sync autoScrollRef/atBottom, so a "X new" badge can appear even when the viewport is already at the latest message. |
SUGGESTION
| File | Line | Issue |
|---|---|---|
desktop/src/apps/chat/MessageList.tsx |
195 | useImperativeHandle invoked without a dependency array; re-creates the handle every render. |
desktop/src/apps/chat/MessageList.tsx |
~329 (scroll container) | The scroll container lost the message-list-drop-target class the original MessagesApp div had. Verify no CSS/JS relied on it (drag highlight is preserved via conditional ring-* classes). |
Files Reviewed (6 files)
desktop/src/apps/MessagesApp.tsx- wiring verified; behavior preserved (ReactionBar, MessageInput, MessageList wired; AttachmentsBar moved into MessageInput; refs replaced byMessageListHandle).desktop/src/apps/chat/ChannelSidebar.tsx- extracted component.desktop/src/apps/chat/MessageInput.tsx- extracted component;onPasteprop added and forwarded to Textarea.desktop/src/apps/chat/MessageList.tsx- 2 issues (see above).desktop/src/apps/chat/ReactionBar.tsx- extracted presentational component.desktop/src/apps/chat/types.ts- shared types.
Reviewed by hy3:free · Input: 50.7K · Output: 1.6K · Cached: 78.4K
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
desktop/src/apps/chat/MessageList.tsx (1)
29-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove shared rendering helpers out of
MessagesApp.tsx.
MessagesApprendersMessageList, while this import points back to the parent module, creating a circular dependency and forcing component tests to load the monolith. Move these helpers into a dependency-neutral chat utility module.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@desktop/src/apps/chat/MessageList.tsx` at line 29, Move renderContent, dayLabel, relativeTime, toMs, and resolveAuthorDisplayState out of MessagesApp.tsx into a dependency-neutral chat utility module, then update MessageList and MessagesApp imports to use that module. Remove the parent-module import from MessageList while preserving each helper’s existing behavior and exports.desktop/src/apps/MessagesApp.tsx (1)
204-212: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove these helpers out of
MessagesAppto break the import cycle.
MessageListconsumes these exports whileMessagesAppimportsMessageList. Put them in a neutral chat utility module so the extracted component does not depend on its parent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@desktop/src/apps/MessagesApp.tsx` around lines 204 - 212, Move the exported toMs and relativeTime helpers out of MessagesApp into a neutral chat utility module, then update both MessagesApp and MessageList imports to use that module. Remove the helper definitions and their exports from MessagesApp so MessageList no longer depends on its parent and the import cycle is eliminated.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@desktop/src/apps/chat/ChannelSidebar.tsx`:
- Around line 502-548: Update the project and project-channel <details> elements
in ChannelSidebar to use the supplied expansion state: bind their open values to
projectsExpanded and projectChannelExpanded, and invoke the corresponding
expansion callbacks from onToggle. Preserve the existing rendering and selection
behavior while ensuring desktop reflects and updates the controlled state.
- Around line 1100-1130: Update the archived actions container around the
restore and delete buttons to use a focus-within visibility utility alongside
group-hover, so the controls become visible when the channel row or its
descendants receive keyboard focus while preserving the existing pointer-hover
behavior.
In `@desktop/src/apps/chat/MessageInput.tsx`:
- Around line 80-88: Update MessageInput’s handleKeyDown and button submission
flow to share a single canSend predicate that accounts for valid uploads rather
than merely a non-empty attachment array. Process mention suggestion selection
and IME composition Enter before routing Enter to onSend, and ensure keyboard
submission uses the same upload gating as the button.
In `@desktop/src/apps/chat/MessageList.tsx`:
- Around line 380-386: Update the message grouping logic in MessageList so
showAuthor is true when showDaySeparator is true, even if the previous message
has the same author; ensure the first message of each new day displays the
author identity alongside the day separator.
In `@desktop/src/apps/MessagesApp.tsx`:
- Around line 1760-1775: Remove the original channel header rendered immediately
above MessageList in the MessagesApp component, leaving MessageList as the sole
owner of the channel header, controls, and pin popover. Preserve the existing
MessageList props and channel behavior.
- Around line 1933-1950: Update the MessageInput usage in MessagesApp to pass
the existing inputRef into the extracted component, and attach that forwarded
ref to MessageInput’s underlying textarea. Preserve the current ref’s use for
focus, cursor-aware mentions, and resizing while eliminating the unrelated
internal-only ref.
- Around line 1584-1586: Update the channel-selection handler passed as
onSelectChannel in MessagesApp so selecting the currently active channel also
clears busSelected, rather than relying only on an effect triggered by
selectedChannel changes. Preserve normal setSelectedChannel behavior for channel
changes while ensuring the bus viewer closes when reselecting the preserved
channel.
---
Nitpick comments:
In `@desktop/src/apps/chat/MessageList.tsx`:
- Line 29: Move renderContent, dayLabel, relativeTime, toMs, and
resolveAuthorDisplayState out of MessagesApp.tsx into a dependency-neutral chat
utility module, then update MessageList and MessagesApp imports to use that
module. Remove the parent-module import from MessageList while preserving each
helper’s existing behavior and exports.
In `@desktop/src/apps/MessagesApp.tsx`:
- Around line 204-212: Move the exported toMs and relativeTime helpers out of
MessagesApp into a neutral chat utility module, then update both MessagesApp and
MessageList imports to use that module. Remove the helper definitions and their
exports from MessagesApp so MessageList no longer depends on its parent and the
import cycle is eliminated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 21bdb5b3-339b-455c-a6c1-8725b66b697e
📒 Files selected for processing (6)
desktop/src/apps/MessagesApp.tsxdesktop/src/apps/chat/ChannelSidebar.tsxdesktop/src/apps/chat/MessageInput.tsxdesktop/src/apps/chat/MessageList.tsxdesktop/src/apps/chat/ReactionBar.tsxdesktop/src/apps/chat/types.ts
| {!scope?.projectId && projectGroups.length > 0 && ( | ||
| <details className="px-3 mt-2"> | ||
| <summary className="cursor-pointer text-[10px] font-semibold uppercase tracking-wider text-white/30 py-1"> | ||
| Projects | ||
| </summary> | ||
| {projectGroups.map((g) => ( | ||
| <details key={g.id} className="ml-2 mt-1"> | ||
| <summary className="cursor-pointer text-xs text-white/60 py-1"> | ||
| {g.name} | ||
| </summary> | ||
| <div className="ml-2 mt-0.5"> | ||
| {g.channels.map((ch) => ( | ||
| <button | ||
| key={ch.id} | ||
| type="button" | ||
| onClick={() => onSelectChannel(ch.id)} | ||
| aria-pressed={selectedChannel === ch.id} | ||
| aria-label={`Channel ${ch.name}`} | ||
| title={ | ||
| ch.settings?.kind === "a2a" | ||
| ? "Agent coordination — mention @<slug> to hand off." | ||
| : undefined | ||
| } | ||
| className={`w-full text-left text-xs py-1 px-2 rounded flex items-center gap-1.5 ${ | ||
| selectedChannel === ch.id | ||
| ? "bg-white/10" | ||
| : "hover:bg-white/5" | ||
| }`} | ||
| > | ||
| {ch.settings?.kind === "a2a" && ( | ||
| <Bot | ||
| size={12} | ||
| aria-hidden | ||
| style={{ | ||
| color: "rgba(255,255,255,0.6)", | ||
| flexShrink: 0, | ||
| }} | ||
| /> | ||
| )} | ||
| {ch.name} | ||
| </button> | ||
| ))} | ||
| </div> | ||
| </details> | ||
| ))} | ||
| </details> | ||
| )} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Honor the supplied project expansion state on desktop.
Both <details> elements are uncontrolled and default closed, ignoring projectsExpanded, projectChannelExpanded, and their callbacks. Wire open/onToggle, or use the controlled button pattern from the mobile implementation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@desktop/src/apps/chat/ChannelSidebar.tsx` around lines 502 - 548, Update the
project and project-channel <details> elements in ChannelSidebar to use the
supplied expansion state: bind their open values to projectsExpanded and
projectChannelExpanded, and invoke the corresponding expansion callbacks from
onToggle. Preserve the existing rendering and selection behavior while ensuring
desktop reflects and updates the controlled state.
| <div className="hidden group-hover:flex items-center shrink-0 pr-1"> | ||
| <button | ||
| type="button" | ||
| onClick={() => | ||
| onRestoreArchivedChannel(ch.id, ch.name) | ||
| } | ||
| disabled={!hasAgent} | ||
| aria-label={`Restore archived channel ${ch.name}`} | ||
| title={ | ||
| hasAgent | ||
| ? "Restore agent" | ||
| : "Agent entry missing — delete only" | ||
| } | ||
| className={`p-1 rounded transition-colors ${ | ||
| hasAgent | ||
| ? "text-white/30 hover:text-emerald-400 hover:bg-emerald-500/10 cursor-pointer" | ||
| : "text-white/15 cursor-not-allowed" | ||
| }`} | ||
| > | ||
| <RotateCcw size={12} aria-hidden="true" /> | ||
| </button> | ||
| <button | ||
| type="button" | ||
| onClick={() => onDeleteArchivedChannel(ch.id)} | ||
| aria-label={`Permanently delete archived channel ${ch.name}`} | ||
| title="Delete permanently" | ||
| className="p-1 rounded text-white/30 hover:text-red-400 hover:bg-red-500/10 transition-colors cursor-pointer" | ||
| > | ||
| <Trash2 size={12} aria-hidden="true" /> | ||
| </button> | ||
| </div> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make archived actions keyboard-accessible.
The restore and delete buttons use hidden group-hover:flex, so they cannot receive focus without a pointer. Reveal them when the channel row has keyboard focus as well.
Proposed fix
- <div className="hidden group-hover:flex items-center shrink-0 pr-1">
+ <div className="hidden group-hover:flex group-focus-within:flex items-center shrink-0 pr-1">📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div className="hidden group-hover:flex items-center shrink-0 pr-1"> | |
| <button | |
| type="button" | |
| onClick={() => | |
| onRestoreArchivedChannel(ch.id, ch.name) | |
| } | |
| disabled={!hasAgent} | |
| aria-label={`Restore archived channel ${ch.name}`} | |
| title={ | |
| hasAgent | |
| ? "Restore agent" | |
| : "Agent entry missing — delete only" | |
| } | |
| className={`p-1 rounded transition-colors ${ | |
| hasAgent | |
| ? "text-white/30 hover:text-emerald-400 hover:bg-emerald-500/10 cursor-pointer" | |
| : "text-white/15 cursor-not-allowed" | |
| }`} | |
| > | |
| <RotateCcw size={12} aria-hidden="true" /> | |
| </button> | |
| <button | |
| type="button" | |
| onClick={() => onDeleteArchivedChannel(ch.id)} | |
| aria-label={`Permanently delete archived channel ${ch.name}`} | |
| title="Delete permanently" | |
| className="p-1 rounded text-white/30 hover:text-red-400 hover:bg-red-500/10 transition-colors cursor-pointer" | |
| > | |
| <Trash2 size={12} aria-hidden="true" /> | |
| </button> | |
| </div> | |
| <div className="hidden group-hover:flex group-focus-within:flex items-center shrink-0 pr-1"> | |
| <button | |
| type="button" | |
| onClick={() => | |
| onRestoreArchivedChannel(ch.id, ch.name) | |
| } | |
| disabled={!hasAgent} | |
| aria-label={`Restore archived channel ${ch.name}`} | |
| title={ | |
| hasAgent | |
| ? "Restore agent" | |
| : "Agent entry missing — delete only" | |
| } | |
| className={`p-1 rounded transition-colors ${ | |
| hasAgent | |
| ? "text-white/30 hover:text-emerald-400 hover:bg-emerald-500/10 cursor-pointer" | |
| : "text-white/15 cursor-not-allowed" | |
| }`} | |
| > | |
| <RotateCcw size={12} aria-hidden="true" /> | |
| </button> | |
| <button | |
| type="button" | |
| onClick={() => onDeleteArchivedChannel(ch.id)} | |
| aria-label={`Permanently delete archived channel ${ch.name}`} | |
| title="Delete permanently" | |
| className="p-1 rounded text-white/30 hover:text-red-400 hover:bg-red-500/10 transition-colors cursor-pointer" | |
| > | |
| <Trash2 size={12} aria-hidden="true" /> | |
| </button> | |
| </div> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@desktop/src/apps/chat/ChannelSidebar.tsx` around lines 1100 - 1130, Update
the archived actions container around the restore and delete buttons to use a
focus-within visibility utility alongside group-hover, so the controls become
visible when the channel row or its descendants receive keyboard focus while
preserving the existing pointer-hover behavior.
| const handleKeyDown = useCallback( | ||
| (e: React.KeyboardEvent) => { | ||
| if (e.key === "Enter" && !e.shiftKey) { | ||
| e.preventDefault(); | ||
| onSend(); | ||
| } | ||
| }, | ||
| [onSend], | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Handle composer state before routing Enter to onSend.
When mention suggestions are open, Enter sends the unfinished draft instead of selecting the highlighted mention, and IME composition Enter can submit partial text. The keyboard path also bypasses upload gating, while a failed attachment incorrectly counts as sendable merely because the array is non-empty.
Use one canSend predicate for both keyboard and button submission, and process mention/IME keys first.
Proposed fix
const textareaRef = useRef<HTMLTextAreaElement>(null);
+ const attachmentsReady = pendingAttachments.every(
+ (attachment) =>
+ Boolean(attachment.record) &&
+ !attachment.uploading &&
+ !attachment.error,
+ );
+ const canSend =
+ !isArchived &&
+ attachmentsReady &&
+ (value.trim().length > 0 || pendingAttachments.length > 0);
const handleKeyDown = useCallback(
(e: React.KeyboardEvent) => {
+ if (e.nativeEvent.isComposing) return;
+
+ if (mention && mentionCandidates.length > 0 && !showSlash) {
+ if (e.key === "ArrowDown" || e.key === "ArrowUp") {
+ e.preventDefault();
+ const delta = e.key === "ArrowDown" ? 1 : -1;
+ onMentionSelChange(
+ (mentionSel + delta + mentionCandidates.length) %
+ mentionCandidates.length,
+ );
+ return;
+ }
+ if (e.key === "Enter") {
+ e.preventDefault();
+ onInsertMention(undefined);
+ return;
+ }
+ if (e.key === "Escape") {
+ e.preventDefault();
+ onDismissMention();
+ return;
+ }
+ }
+
if (e.key === "Enter" && !e.shiftKey) {
e.preventDefault();
- onSend();
+ if (canSend) onSend();
}
},
- [onSend],
+ [
+ canSend,
+ mention,
+ mentionCandidates,
+ mentionSel,
+ onDismissMention,
+ onInsertMention,
+ onMentionSelChange,
+ onSend,
+ showSlash,
+ ],
);
...
- disabled={
- (!value.trim() && pendingAttachments.length === 0) ||
- isArchived ||
- pendingAttachments.some((a) => a.uploading)
- }
+ disabled={!canSend}Also applies to: 119-143, 179-186
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@desktop/src/apps/chat/MessageInput.tsx` around lines 80 - 88, Update
MessageInput’s handleKeyDown and button submission flow to share a single
canSend predicate that accounts for valid uploads rather than merely a non-empty
attachment array. Process mention suggestion selection and IME composition Enter
before routing Enter to onSend, and ensure keyboard submission uses the same
upload gating as the button.
| const prev = i > 0 ? messages[i - 1] : undefined; | ||
| const showAuthor = !prev || prev.author_id !== msg.author_id; | ||
| const prevDay = prev | ||
| ? new Date(toMs(prev.created_at)).toDateString() | ||
| : null; | ||
| const currDay = new Date(toMs(msg.created_at)).toDateString(); | ||
| const showDaySeparator = !prev || prevDay !== currDay; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Show the author again after a day separator.
Consecutive messages from the same author remain grouped across midnight, leaving the first message under the new date without its avatar or name.
Proposed fix
const isAgent = msg.author_type === "agent";
const prev = i > 0 ? messages[i - 1] : undefined;
- const showAuthor = !prev || prev.author_id !== msg.author_id;
const prevDay = prev
? new Date(toMs(prev.created_at)).toDateString()
: null;
const currDay = new Date(toMs(msg.created_at)).toDateString();
const showDaySeparator = !prev || prevDay !== currDay;
+ const showAuthor =
+ !prev || prev.author_id !== msg.author_id || showDaySeparator;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const prev = i > 0 ? messages[i - 1] : undefined; | |
| const showAuthor = !prev || prev.author_id !== msg.author_id; | |
| const prevDay = prev | |
| ? new Date(toMs(prev.created_at)).toDateString() | |
| : null; | |
| const currDay = new Date(toMs(msg.created_at)).toDateString(); | |
| const showDaySeparator = !prev || prevDay !== currDay; | |
| const prev = i > 0 ? messages[i - 1] : undefined; | |
| const prevDay = prev | |
| ? new Date(toMs(prev.created_at)).toDateString() | |
| : null; | |
| const currDay = new Date(toMs(msg.created_at)).toDateString(); | |
| const showDaySeparator = !prev || prevDay !== currDay; | |
| const showAuthor = | |
| !prev || prev.author_id !== msg.author_id || showDaySeparator; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@desktop/src/apps/chat/MessageList.tsx` around lines 380 - 386, Update the
message grouping logic in MessageList so showAuthor is true when
showDaySeparator is true, even if the previous message has the same author;
ensure the first message of each new day displays the author identity alongside
the day separator.
| selectedChannel={selectedChannel} | ||
| onSelectChannel={setSelectedChannel} | ||
| unread={unread} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Clear bus mode directly when selecting a channel.
If bus mode preserves channel A, clicking channel A again makes setSelectedChannel a no-op, so the effect never clears busSelected and the bus viewer remains open.
Proposed fix
- onSelectChannel={setSelectedChannel}
+ onSelectChannel={(id) => {
+ setBusSelected(null);
+ setSelectedChannel(id);
+ }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| selectedChannel={selectedChannel} | |
| onSelectChannel={setSelectedChannel} | |
| unread={unread} | |
| selectedChannel={selectedChannel} | |
| onSelectChannel={(id) => { | |
| setBusSelected(null); | |
| setSelectedChannel(id); | |
| }} | |
| unread={unread} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@desktop/src/apps/MessagesApp.tsx` around lines 1584 - 1586, Update the
channel-selection handler passed as onSelectChannel in MessagesApp so selecting
the currently active channel also clears busSelected, rather than relying only
on an effect triggered by selectedChannel changes. Preserve normal
setSelectedChannel behavior for channel changes while ensuring the bus viewer
closes when reselecting the preserved channel.
| <MessageInput | ||
| value={input} | ||
| onChange={(v) => !isCurrentArchived && handleInputChange(v)} | ||
| onSend={sendMessage} | ||
| channel={currentChannel} | ||
| isArchived={isCurrentArchived} | ||
| isMobile={isMobile} | ||
| keyboardInset={keyboardInset} | ||
| slashCommands={slashCommands} | ||
| showSlash={showSlash} | ||
| slashQuery={slashQuery} | ||
| slashAgent={slashAgent} | ||
| mention={mention} | ||
| mentionCandidates={mentionCandidates} | ||
| mentionSel={mentionSel} | ||
| onMentionSelChange={setMentionSel} | ||
| onInsertMention={insertMention} | ||
| onDismissMention={() => setMention(null)} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reconnect the parent textarea ref.
MessagesApp still uses inputRef for focus, cursor-aware mentions, and resizing, but MessageInput owns an unrelated internal ref and receives no parent ref. Prefill focus now no-ops, and mention detection falls back to assuming the cursor is at the end. Pass and attach the parent ref to the extracted textarea.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@desktop/src/apps/MessagesApp.tsx` around lines 1933 - 1950, Update the
MessageInput usage in MessagesApp to pass the existing inputRef into the
extracted component, and attach that forwarded ref to MessageInput’s underlying
textarea. Preserve the current ref’s use for focus, cursor-aware mentions, and
resizing while eliminating the unrelated internal-only ref.
- Add missing dependency array to useImperativeHandle (was re-creating handle every render) - Remove mount-time auto-scroll useEffect that desyncs autoScrollRef/atBottom - Restore message-list-drop-target class lost during component extraction All three issues from Kilo review resolved. Parent MessagesApp already handles initial scroll via autoScrollRef + scrollToBottom() in its own useEffect.
|
Kilo bot-fix pushed (commit e0fd90f): All 3 Kilo findings on MessageList.tsx resolved:
Vitest suite running (all passing so far). Please re-review. |
- Add missing dependency array to useImperativeHandle (was re-creating handle every render) - Remove mount-time auto-scroll useEffect that desyncs autoScrollRef/atBottom - Restore message-list-drop-target class lost during component extraction All three issues from Kilo review resolved. Parent MessagesApp already handles initial scroll via autoScrollRef + scrollToBottom() in its own useEffect.
e0fd90f to
b00c142
Compare
* test(desktop): add MessageList component tests (Vitest + RTL) Add comprehensive test suite for the extracted MessageList component (PR #1877, upstream #657). Covers: - Empty state / no-channel placeholder - Message rendering with author display - Day separators and new-message divider - Deleted message tombstones - Editing state (MessageEditor) - Pending/streaming/error message states - Edited indicator - Reactions (ReactionBar) - Thread indicators - Dead agent badges (inactive/removed) - Scroll-to-bottom button - Channel header variants (topic/group/dm) - Canvas attachment buttons - Pin request affordance - Typing footer - Pinned messages popover - Mobile keyboard inset - Imperative ref handle (scrollToBottom) 49 tests, all passing. * fix(desktop): address 6 Kilo findings on MessageList tests - Replace vacuous data-reaction/data-typing selectors with real DOM checks - Make day-label assertions locale-independent (query selectors) - Scope reaction/member-count text matchers to avoid ambiguity --------- Co-authored-by: Hogne <227774406+hognek@users.noreply.github.com>
2 participants
Problem
MessagesApp.tsx was the second-largest file in the desktop SPA at 2,856 lines. Extracting sub-components improves testability and maintainability. Closes #657.
What's done (3 commits)
Commit 1 — extract 5 components:
Commit 2 — wire ReactionBar + MessageInput
Commit 3 — wire MessageList (430 lines replaced)
Result: MessagesApp.tsx 2,856 → 2,424 lines (-432, -15%)
Verification
Files changed
6 files, +2,458 / -1,061
Summary by CodeRabbit