Improve web unit tests coverage - #162
Conversation
… to maximize coverage
|
Warning Review limit reached
Next review available in: 22 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: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughThis PR adds comprehensive Jest and React Testing Library coverage for web adapters, hooks, editor utilities, shared components, note management, account actions, public sharing, search interfaces, and export workflows. ChangesWeb test coverage
Estimated code review effort: 4 (Complex) | ~45 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 |
PR StatusUpdated for PR #162 at 📊 Allure Test ReportContributing Workflows
Catalog: All reports 🤖 Android Build PanelCheck a box below to trigger a release build:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
ui/web/tests/unit/components/features/settings/DeleteAccountPanel.test.tsx (1)
107-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert button recovery after failed deletion.
These tests only verify the error text, so they would still pass if
setSubmitting(false)infinallywere removed and the button remained stuck at"Deleting...". Assert that each rejection restores an enabled"Delete account"button.Suggested assertions
await waitFor(() => { expect(screen.getByText("Server error during account deletion")).toBeTruthy() + const button = screen.getByRole("button", { name: "Delete account" }) as HTMLButtonElement + expect(button.disabled).toBe(false) }) ... await waitFor(() => { expect(screen.getByText("Failed to delete account. Please try again.")).toBeTruthy() + const button = screen.getByRole("button", { name: "Delete account" }) as HTMLButtonElement + expect(button.disabled).toBe(false) })As per coding guidelines, tests should cover the changed behavior and all tests should pass before completion.
Also applies to: 122-124
🤖 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 `@ui/web/tests/unit/components/features/settings/DeleteAccountPanel.test.tsx` around lines 107 - 109, Update the failed-deletion test cases around the existing error assertions to also verify recovery: after each rejected deletion, assert that the “Delete account” button is present and enabled rather than remaining in the “Deleting...” state. Cover both rejection paths referenced by the comment while preserving the existing server-error assertions.Source: Coding guidelines
ui/web/tests/unit/lib/editor.test.ts (1)
53-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExact-HTML assertions couple the test to
SmartPasteService's real markdown renderer output.These two tests don't mock
SmartPasteService.resolvePaste(unlike the "bails out" test at Line 88), so they assert on the literal HTML string produced by the real markdown-to-HTML conversion (e.g."<p><strong>Bold Text</strong></p>\n"). Any formatting change in the underlying renderer (whitespace, wrapping, etc.) would break these tests even thoughapplySelectionAsMarkdownitself is unchanged. MockingresolvePastehere too (as done elsewhere in the file) would isolate the unit under test and reduce flakiness risk.♻️ Example: mock resolvePaste for isolation
it("converts selected markdown text to HTML, replaces selection, and triggers onContentChange", () => { const { editor, chainMock, focusMock, deleteRangeMock, insertContentMock, runMock, textBetweenMock, } = createMockEditor(0, 14, "**Bold Text**") const onContentChange = jest.fn() + jest.spyOn(SmartPasteService, "resolvePaste").mockReturnValueOnce({ + html: "<p><strong>Bold Text</strong></p>\n", + type: "markdown", + warnings: [], + detection: { type: "markdown", confidence: 1, reasons: [], warnings: [] }, + }) applySelectionAsMarkdown(editor, onContentChange)🤖 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 `@ui/web/tests/unit/lib/editor.test.ts` around lines 53 - 82, Mock SmartPasteService.resolvePaste in both applySelectionAsMarkdown tests so they verify editor selection replacement and callback behavior without relying on the renderer’s exact HTML output. Configure the mock with representative converted content and update the insertContent assertions to use that controlled result, matching the existing mock pattern in the file.ui/web/tests/unit/components/EditorMenuBar.test.tsx (1)
1-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
isActivemock branching logic is non-obvious; a short comment would help.The three-branch dispatch in
isActiveMock(object-arg vs. name+attributes vs. plain name) mirrors TipTap's overloadededitor.isActive()signature, but this isn't explained anywhere. A short comment mapping each branch to its real-world call site (e.g.editor.isActive({ textAlign: 'left' })vseditor.isActive('heading', { level })vseditor.isActive('bold')) would make future maintenance easier.As per coding guidelines, "Follow established project code style, use meaningful self-documenting names, and add comments for complex or non-obvious logic."
🤖 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 `@ui/web/tests/unit/components/EditorMenuBar.test.tsx` around lines 1 - 129, Add a concise comment immediately above isActiveMock's branching logic explaining that it mirrors TipTap's overloaded editor.isActive calls: object argument for attributes such as textAlign, name plus attributes for nodes such as heading levels, and plain name for marks such as bold. Leave the existing branching behavior unchanged.Source: Coding guidelines
ui/web/tests/unit/components/features/search/NoteSearchResults.test.tsx (1)
126-141: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid hardcoding derived chunk length; matches inconsistency with sibling test file.
Line 140 hardcodes
24as the expectedchunkLengthargument, coupling the test to the exact output ofgetRagChunkBodyLength.ChunkSearchItem.test.tsxusesexpect.any(Number)for the equivalent assertion — consider the same here for consistency and resilience to unrelated implementation changes.♻️ Suggested change
- expect(mockOnOpenInContext).toHaveBeenCalledWith('note-42', 10, 24) + expect(mockOnOpenInContext).toHaveBeenCalledWith('note-42', 10, expect.any(Number))🤖 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 `@ui/web/tests/unit/components/features/search/NoteSearchResults.test.tsx` around lines 126 - 141, Update the onOpenInContext assertion in the “invokes onOpenInContext callback when clicking open fragment” test to avoid hardcoding the derived chunk length; keep the expected note ID and start offset unchanged, and match the sibling ChunkSearchItem test by accepting any numeric value for the final argument.ui/web/tests/unit/hooks/useTagSuggestions.test.ts (1)
43-55: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant/confusing first assertion.
['rub', 'rust'].filter((t) => defaultTags.includes(t))always evaluates to['rust'](since'rub'isn't indefaultTags), so it's equivalent to the very next assertion. It adds no coverage and obscures what's actually being tested.♻️ Proposed simplification
- expect(result.current).toEqual(['rub', 'rust'].filter((t) => defaultTags.includes(t))) expect(result.current).toEqual(['rust'])🤖 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 `@ui/web/tests/unit/hooks/useTagSuggestions.test.ts` around lines 43 - 55, Remove the redundant first expect in the custom minChars test and retain the direct assertion that result.current equals ['rust']. Keep the test setup and minChars behavior unchanged.ui/web/tests/unit/hooks/use-toast.test.tsx (1)
264-285: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest doesn't actually verify duplicate-timer prevention.
The final assertions (
toasts[0].open === false, thentoasts.length === 0after advancing timers) would pass identically whether or not a dedup guard exists for the removal timeout —REMOVE_TOASTfiltering by id is a no-op on an already-removed id either way. This test can't distinguish "no duplicate timer" from "duplicate timer without a guard."♻️ Proposed fix using timer-count assertion
act(() => { result.current.dismiss(createdId) result.current.dismiss(createdId) }) + // Only one removal timer should have been scheduled despite two dismiss calls + expect(jest.getTimerCount()).toBe(1) + expect(result.current.toasts[0].open).toBe(false)🤖 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 `@ui/web/tests/unit/hooks/use-toast.test.tsx` around lines 264 - 285, Strengthen the duplicate-dismiss test around useToast().dismiss by asserting the scheduled removal timer count immediately after calling dismiss twice. Verify that only one removal timer is created for the toast, while retaining the existing state and eventual-removal assertions.ui/web/tests/unit/components/ImportProgressDialog.test.tsx (1)
157-176: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winFragile button disambiguation via
textContent.
closeButtons.find(btn => btn.textContent === "Close")doesn't reliably distinguish the footer Close button from a default dialog close icon (its sr-only "Close" text would produce the sametextContent). It currently works only because of coincidental DOM ordering. The same file's other test (Line 182) uses a more robust!btn.querySelector("svg")filter for the same disambiguation — consider aligning this test with that pattern.♻️ Suggested alignment
- const footerBtn = closeButtons.find(btn => btn.textContent === "Close")! + const footerBtn = closeButtons.find(btn => !btn.querySelector("svg"))!🤖 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 `@ui/web/tests/unit/components/ImportProgressDialog.test.tsx` around lines 157 - 176, Update the Close-button selection in the “renders Close button when import is complete and calls onClose on click” test to exclude buttons containing an SVG, matching the existing robust disambiguation pattern in the nearby test. Remove reliance on exact textContent or DOM ordering while preserving the click and onClose assertion.ui/web/tests/unit/components/VirtualNoteList.test.tsx (1)
255-301: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueRemove the second-argument assertion from this mock expectation.
Function components receive props as their first argument; any remaining positional arguments are a React implementation detail and should not be part of this contract.
♻️ Suggested simplification
- expect(customListSpy).toHaveBeenCalledWith( - expect.objectContaining({ - height: 400, - itemCount: 1, - itemSize: 120, - width: "100%", - overscanCount: 5, - }), - undefined - ) + expect(customListSpy.mock.calls[0][0]).toEqual( + expect.objectContaining({ + height: 400, + itemCount: 1, + itemSize: 120, + width: "100%", + overscanCount: 5, + }) + )🤖 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 `@ui/web/tests/unit/components/VirtualNoteList.test.tsx` around lines 255 - 301, Update both customListSpy expectations in the “passes custom height and virtualizer parameters to ListComponent” test to assert only the props object passed as the first argument; remove the undefined second-argument assertions while preserving all existing prop checks.
🤖 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 `@ui/web/tests/unit/components/AuthForm.test.tsx`:
- Around line 115-135: Update the test around AuthForm’s Google authentication
click to make onGoogleAuthMock reject, so it genuinely exercises the failure
path and verifies the button’s loading state is reset. Remove the unnecessary
try/catch around fireEvent.click, while preserving the existing call-count
assertion and console-error cleanup.
In `@ui/web/tests/unit/components/features/notes/MoreActionsMenu.test.tsx`:
- Around line 136-142: Fix the RAG-panel close test by updating the DropdownMenu
mock to respect its open prop and expose observable closed-state behavior, then
assert that clicking close-menu-from-rag causes the menu to close through the
onMenuClose/setMoreMenuOpen(false) wiring. Replace the tautological closeBtn
existence assertion in the supports closing menu via RAG panel callback test
with an assertion against the mock’s rendered open state.
In `@ui/web/tests/unit/components/features/public/PublicSharePageClient.test.tsx`:
- Around line 136-151: Strengthen the “cancels state updates cleanly when
unmounted before request resolves” test by spying on the relevant React warning
mechanism, resolving the pending request after unmount, and asserting that no
post-unmount state-update warning is emitted. Restore the spy afterward while
preserving the existing unmount-before-resolution flow.
In
`@ui/web/tests/unit/components/features/wordpress/ExportToWordPressButton.test.tsx`:
- Around line 65-81: Expand the test named “does not call onRequestExport if
getNote returns null or note without id” to cover both guard inputs: retain the
null case and add a separate note object without an id, such as one containing
only a title. Verify in each case that getNote is called once and
onRequestExport is not called.
---
Nitpick comments:
In `@ui/web/tests/unit/components/EditorMenuBar.test.tsx`:
- Around line 1-129: Add a concise comment immediately above isActiveMock's
branching logic explaining that it mirrors TipTap's overloaded editor.isActive
calls: object argument for attributes such as textAlign, name plus attributes
for nodes such as heading levels, and plain name for marks such as bold. Leave
the existing branching behavior unchanged.
In `@ui/web/tests/unit/components/features/search/NoteSearchResults.test.tsx`:
- Around line 126-141: Update the onOpenInContext assertion in the “invokes
onOpenInContext callback when clicking open fragment” test to avoid hardcoding
the derived chunk length; keep the expected note ID and start offset unchanged,
and match the sibling ChunkSearchItem test by accepting any numeric value for
the final argument.
In `@ui/web/tests/unit/components/features/settings/DeleteAccountPanel.test.tsx`:
- Around line 107-109: Update the failed-deletion test cases around the existing
error assertions to also verify recovery: after each rejected deletion, assert
that the “Delete account” button is present and enabled rather than remaining in
the “Deleting...” state. Cover both rejection paths referenced by the comment
while preserving the existing server-error assertions.
In `@ui/web/tests/unit/components/ImportProgressDialog.test.tsx`:
- Around line 157-176: Update the Close-button selection in the “renders Close
button when import is complete and calls onClose on click” test to exclude
buttons containing an SVG, matching the existing robust disambiguation pattern
in the nearby test. Remove reliance on exact textContent or DOM ordering while
preserving the click and onClose assertion.
In `@ui/web/tests/unit/components/VirtualNoteList.test.tsx`:
- Around line 255-301: Update both customListSpy expectations in the “passes
custom height and virtualizer parameters to ListComponent” test to assert only
the props object passed as the first argument; remove the undefined
second-argument assertions while preserving all existing prop checks.
In `@ui/web/tests/unit/hooks/use-toast.test.tsx`:
- Around line 264-285: Strengthen the duplicate-dismiss test around
useToast().dismiss by asserting the scheduled removal timer count immediately
after calling dismiss twice. Verify that only one removal timer is created for
the toast, while retaining the existing state and eventual-removal assertions.
In `@ui/web/tests/unit/hooks/useTagSuggestions.test.ts`:
- Around line 43-55: Remove the redundant first expect in the custom minChars
test and retain the direct assertion that result.current equals ['rust']. Keep
the test setup and minChars behavior unchanged.
In `@ui/web/tests/unit/lib/editor.test.ts`:
- Around line 53-82: Mock SmartPasteService.resolvePaste in both
applySelectionAsMarkdown tests so they verify editor selection replacement and
callback behavior without relying on the renderer’s exact HTML output. Configure
the mock with representative converted content and update the insertContent
assertions to use that controlled result, matching the existing mock pattern in
the file.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d67e04d6-64a5-4500-8cfd-3f4952bcf9c5
📒 Files selected for processing (24)
ui/web/tests/unit/adapters/networkStatus.test.tsui/web/tests/unit/components/AuthForm.test.tsxui/web/tests/unit/components/EditorMenuBar.test.tsxui/web/tests/unit/components/ErrorBoundary.test.tsxui/web/tests/unit/components/ExportProgressDialog.test.tsxui/web/tests/unit/components/ImportProgressDialog.test.tsxui/web/tests/unit/components/VirtualNoteList.test.tsxui/web/tests/unit/components/executeEditorCommand.test.tsui/web/tests/unit/components/features/account/DeleteAccountDialog.test.tsxui/web/tests/unit/components/features/notes/MoreActionsMenu.test.tsxui/web/tests/unit/components/features/notes/Sidebar.test.tsxui/web/tests/unit/components/features/public/PublicSharePageClient.test.tsxui/web/tests/unit/components/features/search/AiSearchPresetSelector.test.tsxui/web/tests/unit/components/features/search/ChunkSearchItem.test.tsxui/web/tests/unit/components/features/search/NoteSearchResults.test.tsxui/web/tests/unit/components/features/settings/DeleteAccountPanel.test.tsxui/web/tests/unit/components/features/wordpress/ExportToWordPressButton.test.tsxui/web/tests/unit/components/theme-toggle.test.tsxui/web/tests/unit/hooks/use-mobile.test.tsxui/web/tests/unit/hooks/use-toast.test.tsxui/web/tests/unit/hooks/useNoteData.test.tsxui/web/tests/unit/hooks/useNotesQuery.test.tsxui/web/tests/unit/hooks/useTagSuggestions.test.tsui/web/tests/unit/lib/editor.test.ts
… build validation
|
Updated branch with fixes for CodeRabbit feedback, SonarQube issues, and build validation. |
|
1 participant
… to maximize coverage
Summary by CodeRabbit