tsk-55xqwq [OPEN] Land #2167: rebase the duplicate-channel-header fi - #2169
Conversation
The channel header block in MessagesApp.tsx (type icon, agent emoji, channel name, settings, guide link, pin/search, member count, description) was a near-exact copy of the header already rendered by MessageList.tsx at line 214. Since messageAreaUI renders both its own header and <MessageList>, the result was two stacked headers. Remove the duplicate header from MessagesApp.tsx and clean up the five imports that became unused (MessagesSquare, Search, PinBadge, PinnedMessagesPopover, resolveAgentEmoji). The PinnedMessage type import is preserved. Closes #2124
📝 WalkthroughWalkthroughMessagesApp updates its icon and type imports, removes the ChangesMessagesApp updates
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
PR Summary by QodoFix stacked channel headers by removing duplicate header in MessagesApp
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
|
nemotron-ultra-orB review VERDICT: Significant UI removal without clear replacement; likely dead code and unused state remain.
Automated first-pass review by the nemotron-ultra-orB lane. The lead still reviews before merge. |
|
nemotron-ultra-kilo review VERDICT: Major functionality removed without clear relocation — channel header, pinned messages, search, and thread navigation features appear deleted.
Automated first-pass review by the nemotron-ultra-kilo lane. The lead still reviews before merge. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
desktop/src/apps/MessagesApp.tsx (1)
1698-1703: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd the requested regression coverage before merging.
Render a selected channel and assert that exactly one channel header is present, while pin, thread, search, and settings actions remain available. The PR objective explicitly says this test acceptance criterion must be resolved before merge.
🤖 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 1698 - 1703, Add regression coverage for the MessagesApp rendering path around MessageList: render a selected channel and assert exactly one channel header is present, while verifying pin, thread, search, and settings actions remain available.
🤖 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.
Outside diff comments:
In `@desktop/src/apps/MessagesApp.tsx`:
- Around line 1698-1703: Add regression coverage for the MessagesApp rendering
path around MessageList: render a selected channel and assert exactly one
channel header is present, while verifying pin, thread, search, and settings
actions remain available.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: bc900eee-6221-4962-b3cb-f909fd0b7809
⛔ Files ignored due to path filters (1)
desktop/package-lock.jsonis excluded by!**/package-lock.json,!**/package-lock.json
📒 Files selected for processing (1)
desktop/src/apps/MessagesApp.tsx
Code Review by Qodo
1. MessagesApp missing regression test
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| {/* channel header — MobileSplitView owns back nav on mobile */} | ||
| <div className="px-4 py-2.5 border-b border-shell-border flex items-center gap-3 shrink-0"> |
There was a problem hiding this comment.
1. Messagesapp missing regression test 📜 Skill insight ▣ Testability
This PR changes MessagesApp behavior by removing the channel header UI but does not add a regression test to prevent the duplicate-header bug from returning. Without a test, future refactors can reintroduce the issue undetected.
Agent Prompt
## Issue description
A UI bug fix was made in `MessagesApp` (removing the duplicate channel header), but no regression test was added.
## Issue Context
Compliance requires every bug fix PR to include at least one test that would fail without the fix and pass with it.
## Fix Focus Areas
- desktop/src/apps/MessagesApp.tsx[1699-1706]
- desktop/src/apps/chat/__tests__/MessageList.test.tsx[121-160]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| onJumpTo={(id) => { | ||
| setPinnedPopoverOpen(false); | ||
| const el = document.querySelector(`[data-message-id="${id}"]`) as HTMLElement | null; | ||
| if (el) { | ||
| el.scrollIntoView({ behavior: "smooth", block: "center" }); | ||
| el.classList.add("data-highlight"); | ||
| setTimeout(() => el.classList.remove("data-highlight"), 2000); | ||
| } else { | ||
| // Only ~50 messages load; a pin older than that is not in the DOM. | ||
| setSendError("Message is older than the loaded history"); | ||
| } |
There was a problem hiding this comment.
2. Silent pin jump failure 🐞 Bug ≡ Correctness
After removing the pinned-messages popover from MessagesApp, jumping to a pinned message that isn’t currently in the DOM now no-ops with no user feedback. Users clicking an older pin may see nothing happen even though MessagesApp still has a visible error banner mechanism (sendError) that previously handled this case.
Agent Prompt
### Issue description
The pinned-message jump handler that used to surface an error (when the target message element is not found in the DOM) was removed from `MessagesApp.tsx`. The remaining implementation inside `MessageList` only scrolls when the element exists and provides no fallback feedback.
### Issue Context
- `MessageList` currently does `document.querySelector(...)` and scrolls/highlights only if the element exists.
- `MessagesApp` still renders a `sendError` banner, but the remaining pin-jump path no longer triggers it.
### Fix Focus Areas
- Add an explicit “not found” branch for pin jumps and surface an error to the user (e.g., via an optional callback prop that `MessagesApp` implements by calling `setSendError(...)`).
- Keep the current success behavior (scroll + highlight) unchanged.
#### Code locations
- desktop/src/apps/chat/MessageList.tsx[63-146]
- desktop/src/apps/chat/MessageList.tsx[259-281]
- desktop/src/apps/MessagesApp.tsx[1701-1774]
- desktop/src/apps/MessagesApp.tsx[1849-1854]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (1 file)
Reviewed by step-3.7-flash · Input: 83.7K · Output: 17.3K · Cached: 1.5M |
…er fi (jaylfc#2169) * fix(messages): remove duplicate channel header causing stacked rendering The channel header block in MessagesApp.tsx (type icon, agent emoji, channel name, settings, guide link, pin/search, member count, description) was a near-exact copy of the header already rendered by MessageList.tsx at line 214. Since messageAreaUI renders both its own header and <MessageList>, the result was two stacked headers. Remove the duplicate header from MessagesApp.tsx and clean up the five imports that became unused (MessagesSquare, Search, PinBadge, PinnedMessagesPopover, resolveAgentEmoji). The PinnedMessage type import is preserved. Closes jaylfc#2124 * tsk-55xqwq [OPEN] Land jaylfc#2167: rebase the duplicate-channel-header fi --------- Co-authored-by: Hogne <227774406+hognek@users.noreply.github.com>
…er fi (jaylfc#2169) * fix(messages): remove duplicate channel header causing stacked rendering The channel header block in MessagesApp.tsx (type icon, agent emoji, channel name, settings, guide link, pin/search, member count, description) was a near-exact copy of the header already rendered by MessageList.tsx at line 214. Since messageAreaUI renders both its own header and <MessageList>, the result was two stacked headers. Remove the duplicate header from MessagesApp.tsx and clean up the five imports that became unused (MessagesSquare, Search, PinBadge, PinnedMessagesPopover, resolveAgentEmoji). The PinnedMessage type import is preserved. Closes jaylfc#2124 * tsk-55xqwq [OPEN] Land jaylfc#2167: rebase the duplicate-channel-header fi --------- Co-authored-by: Hogne <227774406+hognek@users.noreply.github.com>
2 participants
Autonomous build of board card tsk-55xqwq.
Files:
desktop/src/apps/MessagesApp.tsx | 123 +--------------------------------------
1 file changed, 1 insertion(+), 122 deletions(-)
Summary by CodeRabbit