test(chat): add 27 vitest tests for ThreadPanel component#2053
Conversation
Covers rendering (normal/fullscreen layouts), parent message fetch & display, reply fetch & display (including de-duplication of liveReplies), load errors (parent + replies failure), send errors, submit behavior (Enter/Shift+Enter/empty input), input clearing on success/failure, disabled-while-sending state, and close button (normal + fullscreen). Task: t_4c5477ca. Ref: jaylfc#657
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: 32 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}
| }); | ||
|
|
||
| it("scrolls to bottom when a new liveReply arrives", async () => { | ||
| stubFetchOk(); |
There was a problem hiding this comment.
SUGGESTION: Test name says "scrolls to bottom" but asserts only that the reply rendered, not the scroll position.
The component scrolls via scrollRef.current.scrollTop = scrollRef.current.scrollHeight (ThreadPanel.tsx:57) in an effect keyed on liveReplies.length. This behavior is never actually verified. Either assert the scroll position (e.g. read the scroll container's scrollTop/scrollHeight after rerender) or rename the test so it doesn't over-promise coverage.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| vi.stubGlobal( | ||
| "fetch", | ||
| vi.fn().mockImplementation((url: string) => { | ||
| // Match on the LAST path segment that distinguishes the endpoints: |
There was a problem hiding this comment.
SUGGESTION: The route-matching comments are inaccurate and a maintenance hazard.
The comment claims it matches on the "LAST path segment" / uses url.endsWith, but the code uses url.includes("/threads/") then url.includes("/messages/"). The replies URL /api/chat/channels/{ch}/threads/{id}/messages contains BOTH segments, and it maps to "threads" only because /threads/ is checked first. The documented logic does not match the implementation. If someone reorders the if branches or changes the URL shape, the stub will silently break. Correct the comment to describe the actual includes-based, order-dependent matching.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| created_at: 1700000200, | ||
| }; | ||
|
|
||
| beforeEach(() => { |
There was a problem hiding this comment.
SUGGESTION: beforeEach only calls vi.restoreAllMocks(), which does not reset a vi.stubGlobal("fetch", ...) left over from a test that throws before afterEach runs vi.unstubAllGlobals().
Most tests rely on afterEach to unstub globals, so this works in practice. But if a test throws mid-flight, a leaked global fetch stub could bleed into a subsequent test and cause confusing failures. Consider adding vi.unstubAllGlobals() to beforeEach as a defensive reset.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| expect(onSend).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("does NOT submit on empty or whitespace-only input", () => { |
There was a problem hiding this comment.
SUGGESTION: Coverage gap: the sending re-entrancy guard in submit() (if (!content || sending) return, ThreadPanel.tsx:89) is not tested.
The "disables textarea while sending" test confirms the disabled state changes, but no test verifies that a second Enter keypress mid-send is ignored (i.e. onSend is still called only once while sending is true). Since the component explicitly guards against double-submit, an assertion like expect(onSend).toHaveBeenCalledTimes(1) after firing a second Enter while the send promise is pending would lock in that behavior.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: No Issues Found (prior 4 suggestions resolved) | Recommendation: Merge Overview
Previously Reported (now resolved in this update)The incremental diff (
Files Reviewed (1 file)
Previous Review Summary (commit 6be787e)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 6be787e)Status: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (1 files)
Reviewed by hy3:free · Input: 36.4K · Output: 2.6K · Cached: 166.9K |
- Strengthen scroll test with actual scrollTop assertion against pinned scrollHeight - Fix route-matching comments: code uses includes, not endsWith - Add unstubAllGlobals to beforeEach for defensive global state reset - Add re-entrancy test verifying second Enter is ignored while sending
2 participants
Task: t_4c5477ca. Fixes #657 (C4).
Adds comprehensive Vitest + React Testing Library tests for the ThreadPanel
component extracted from MessagesApp.tsx.
Coverage (27 tests):
All 97 existing chat tests + 27 new = 124 passing. tsc --noEmit clean.