test(desktop): add MessageList component tests (Vitest + RTL)#2052
Conversation
Add comprehensive test suite for the extracted MessageList component (PR jaylfc#1877, upstream jaylfc#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.
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? |
|
Warning Review limit reached
Next review available in: 5 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)
✨ 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 |
|
|
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| }); | ||
| // No reaction elements | ||
| const reactionButtons = document.querySelectorAll("[data-reaction]"); | ||
| expect(reactionButtons.length).toBe(0); |
There was a problem hiding this comment.
WARNING: Vacuous assertion — this test can never fail.
ReactionBar (ReactionBar.tsx) does not attach a data-reaction attribute to its buttons, so document.querySelectorAll("[data-reaction]") is always empty. With reactions: {} the ReactionBar is correctly skipped by the component (it guards on Object.keys(msg.reactions).length > 0), but this assertion would also return 0 if ReactionBar were rendered unconditionally. It does not actually verify that no reactions are rendered. Assert on the absence of the emoji text or the ReactionBar's rendered structure instead (e.g. expect(screen.queryByText("👍")).toBeNull()).
| />, | ||
| ); | ||
| // TypingFooter returns null when empty, so no typing text | ||
| expect(container.querySelector("[data-typing]")).toBeNull(); |
There was a problem hiding this comment.
WARNING: Vacuous assertion — this test can never fail.
TypingFooter (TypingFooter.tsx) returns null when no one is typing, but it never renders an element with a data-typing attribute regardless. container.querySelector("[data-typing]") is therefore always null, so this test passes even if TypingFooter rendered broken/placeholder markup. It does not validate empty-state behavior. Assert on the documented contract instead — e.g. that the typing text is absent: expect(screen.queryByText(/typing/i)).toBeNull() (noting the component filters selfId).
| messages: [msg()], | ||
| }); | ||
| // dayLabel returns "Today", "Yesterday", or a date string like "Tue, Nov 14" | ||
| expect(screen.getByText(/Today|Yesterday|[A-Z][a-z]{2}, [A-Z][a-z]{2} \d{1,2}/)).toBeInTheDocument(); |
There was a problem hiding this comment.
SUGGESTION: Locale-dependent day-label assertion.
dayLabel (MessagesApp.tsx:315) formats past dates via toLocaleDateString(undefined, { weekday: "short", day: "numeric", month: "short" }), whose output depends on the test runner's locale (e.g. en-US yields "Tue, Nov 14", but other locales yield different orders/separators). This regex assumes the en-US Weekday, Mon DD shape and will fail on CI runners configured with a non-English locale. Pin the locale (e.g. set Intl/process.env.LANG in the test setup) or assert on a stable, locale-independent signal.
| ], | ||
| }); | ||
| // Should have two day labels | ||
| const separators = screen.getAllByText(/Today|Yesterday|[A-Z][a-z]{2}, [A-Z][a-z]{2} \d{1,2}/); |
There was a problem hiding this comment.
SUGGESTION: Same locale-dependence as the day-label test above. The toLocaleDateString output in dayLabel is locale-specific, so the length === 2 result is correct only under an en-US locale. Pin the locale in test setup or rely on a locale-independent check so this stays green on any CI image.
| renderWithMsg({ | ||
| channel: channel({ type: "group", members: ["user", "alice", "bob"] }), | ||
| }); | ||
| expect(screen.getByText("3")).toBeInTheDocument(); |
There was a problem hiding this comment.
SUGGESTION: Brittle text matcher.
screen.getByText("3") for the member count matches any element whose text is exactly "3". It currently passes, but it will break (or worse, produce a confusing multiple-match error) if any other text in the header ever renders "3" (e.g. a future count badge). Prefer scoping the query, e.g. screen.getByText("3", { selector: "div" }) or asserting within the members container.
| }); | ||
| // ReactionBar should show the emoji and count | ||
| expect(screen.getByText("👍")).toBeInTheDocument(); | ||
| expect(screen.getByText("1")).toBeInTheDocument(); |
There was a problem hiding this comment.
SUGGESTION: Ambiguous / brittle count matcher.
screen.getByText("1") targets the reaction count, but "1" is a very common text fragment and this will throw a multiple-element error if any other "1" appears in the rendered tree (e.g. a future thread-reply count, new-message count, or relative-time value). Scope it or use a more specific query, e.g. assert the ReactionBar button's structure directly.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Resolution Notes (incremental review)All 6 findings from the previous review (commit
No new issues were introduced by the incremental changes. Files Reviewed (1 files)
Previous Review Summary (commit 299d1c1)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 299d1c1)Status: 6 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (1 files)
Reviewed by hy3:free · Input: 37.2K · Output: 2.6K · Cached: 133.6K |
- 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
2 participants
Task: t_b86ff267. Closes #657 (C1).
Adds comprehensive Vitest + React Testing Library tests for the extracted MessageList component (from PR #1877).
Coverage:
49 tests, all passing.
npm run buildsucceeds.tsc -bclean.