test: React Testing Library + renderer component tests, coverage to 33% (Phase 3) - #3365
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (23)
WalkthroughAdds React Testing Library ( ChangesRTL Test Infrastructure and Component Specs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/ui/components/DownloadsManagerView/index.spec.tsx`:
- Around line 152-171: The test 'clears the search filter when the clear-all
button is clicked' currently uses user.type to populate the search field, but
this approach is unstable in this renderer as documented. Instead, seed the
download-search state directly in the preloadedState object passed to
renderWithStore with an initial value of 'beta', then remove the user.type call
entirely. This way the test verifies the actual persisted-filter clearing
behavior without relying on typing interactions, making it deterministic and
testing the real code path.
In `@src/ui/components/utils/createAnchor.spec.ts`:
- Around line 5-7: Remove the manual document.body.innerHTML = '' cleanup from
the afterEach hooks in createAnchor.spec.ts, as this can orphan portal anchors
and cause NotFoundError crashes during RTL cleanup. Instead, rely on React
Testing Library's automatic cleanup by removing these afterEach blocks entirely,
or use component-level cleanup methods like unmount() where needed. This applies
to multiple afterEach hooks in the file (including the ones around lines 5-7 and
54-56).
In `@src/ui/components/utils/ReparentingContainer.spec.tsx`:
- Around line 37-39: The test file is using a type cast to work around a type
mismatch in the ReparentingContainer component. The root issue is that
ReparentingContainerProps does not declare that the component accepts HTML div
attributes, even though it spreads extra props onto its inner div element.
Update the ReparentingContainerProps type definition to extend
React.HTMLAttributes<HTMLDivElement> in addition to its existing properties,
then remove the unnecessary type cast on lines 37-39 where Container is defined
in the test 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aeede7c0-fe85-45b3-906e-c2d1feefd02e
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (23)
docs/KNOWN_ISSUES.mdjest.config.jspackage.jsonsrc/.jest/setup.tssrc/jest-dom.d.tssrc/logging/main/index.main.spec.tssrc/screenSharing/main/ScreenSharingRequestTracker.main.spec.tssrc/screenSharing/main/desktopCapturerCache.main.spec.tssrc/ui/components/AddServerView/index.spec.tsxsrc/ui/components/ClearCacheDialog/index.spec.tsxsrc/ui/components/DownloadsManagerView/DownloadItem.spec.tsxsrc/ui/components/DownloadsManagerView/index.spec.tsxsrc/ui/components/Modal/ModalBackdrop.spec.tsxsrc/ui/components/OutlookCredentialsDialog/index.spec.tsxsrc/ui/components/ScreenSharingDialog/index.spec.tsxsrc/ui/components/Shell/index.spec.tsxsrc/ui/components/SideBar/ServerButton.spec.tsxsrc/ui/components/SideBar/index.spec.tsxsrc/ui/components/SupportedVersionDialog/index.spec.tsxsrc/ui/components/UpdateDialog/index.spec.tsxsrc/ui/components/utils/ReparentingContainer.spec.tsxsrc/ui/components/utils/createAnchor.spec.tssrc/ui/test-utils.tsx
📜 Review details
🧰 Additional context used
📓 Path-based instructions (6)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Use TypeScript for all new code unless explicitly told otherwise
Use optional chaining with fallbacks for platform-specific APIs instead of mocking when possible. Example:const uid = process.getuid?.() ?? 1000;
Files:
src/jest-dom.d.tssrc/screenSharing/main/desktopCapturerCache.main.spec.tssrc/screenSharing/main/ScreenSharingRequestTracker.main.spec.tssrc/ui/components/utils/createAnchor.spec.tssrc/logging/main/index.main.spec.ts
**/*.{tsx,ts}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{tsx,ts}: MANDATORY: Use Fuselage components for all UI work. Only create custom components when Fuselage doesn't provide what's needed
Import UI components from@rocket.chat/fuselageand checkTheme.d.tsfor valid color tokens
Use React functional components with hooks
Use PascalCase for component file names
Files:
src/jest-dom.d.tssrc/ui/components/ScreenSharingDialog/index.spec.tsxsrc/ui/components/OutlookCredentialsDialog/index.spec.tsxsrc/screenSharing/main/desktopCapturerCache.main.spec.tssrc/ui/components/DownloadsManagerView/DownloadItem.spec.tsxsrc/ui/components/Modal/ModalBackdrop.spec.tsxsrc/ui/components/SupportedVersionDialog/index.spec.tsxsrc/screenSharing/main/ScreenSharingRequestTracker.main.spec.tssrc/ui/components/utils/createAnchor.spec.tssrc/ui/components/UpdateDialog/index.spec.tsxsrc/logging/main/index.main.spec.tssrc/ui/components/SideBar/ServerButton.spec.tsxsrc/ui/components/Shell/index.spec.tsxsrc/ui/components/AddServerView/index.spec.tsxsrc/ui/components/ClearCacheDialog/index.spec.tsxsrc/ui/components/DownloadsManagerView/index.spec.tsxsrc/ui/components/utils/ReparentingContainer.spec.tsxsrc/ui/test-utils.tsxsrc/ui/components/SideBar/index.spec.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Redux actions must follow FSA (Flux Standard Action) pattern
Avoid unnecessary comments — write self-documenting code through clear naming
Always verify libraries by checking official docs and.d.tsfiles innode_modules/. Never assume props, tokens, or APIs work without verification
Avoid subjective descriptors ('smart', 'excellent', 'dumb') in documentation and comments
Use measurable descriptions in code documentation: 'reduced memory usage', 'improved by X%' instead of subjective claims
NEVER invent metrics — don't include estimated time spent or speculated user counts. Only include numbers from actual logs, error messages, or documented sources
Files:
src/jest-dom.d.tssrc/ui/components/ScreenSharingDialog/index.spec.tsxsrc/ui/components/OutlookCredentialsDialog/index.spec.tsxsrc/screenSharing/main/desktopCapturerCache.main.spec.tssrc/ui/components/DownloadsManagerView/DownloadItem.spec.tsxsrc/ui/components/Modal/ModalBackdrop.spec.tsxsrc/ui/components/SupportedVersionDialog/index.spec.tsxsrc/screenSharing/main/ScreenSharingRequestTracker.main.spec.tssrc/ui/components/utils/createAnchor.spec.tssrc/ui/components/UpdateDialog/index.spec.tsxsrc/logging/main/index.main.spec.tssrc/ui/components/SideBar/ServerButton.spec.tsxsrc/ui/components/Shell/index.spec.tsxsrc/ui/components/AddServerView/index.spec.tsxsrc/ui/components/ClearCacheDialog/index.spec.tsxsrc/ui/components/DownloadsManagerView/index.spec.tsxsrc/ui/components/utils/ReparentingContainer.spec.tsxsrc/ui/test-utils.tsxsrc/ui/components/SideBar/index.spec.tsx
**/*.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
*.spec.tsfile naming for Renderer process tests
Files:
src/screenSharing/main/desktopCapturerCache.main.spec.tssrc/screenSharing/main/ScreenSharingRequestTracker.main.spec.tssrc/ui/components/utils/createAnchor.spec.tssrc/logging/main/index.main.spec.ts
**/*.main.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
*.main.spec.tsfile naming for Main process tests
Files:
src/screenSharing/main/desktopCapturerCache.main.spec.tssrc/screenSharing/main/ScreenSharingRequestTracker.main.spec.tssrc/logging/main/index.main.spec.ts
**/*.{spec.ts,main.spec.ts}
📄 CodeRabbit inference engine (CLAUDE.md)
Only mock platform-specific APIs when defensive coding isn't possible. Linux-only APIs requiring mocks:
process.getuid(),process.getgid(),process.geteuid(),process.getegid()
Files:
src/screenSharing/main/desktopCapturerCache.main.spec.tssrc/screenSharing/main/ScreenSharingRequestTracker.main.spec.tssrc/ui/components/utils/createAnchor.spec.tssrc/logging/main/index.main.spec.ts
🪛 ast-grep (0.44.0)
src/ui/components/utils/createAnchor.spec.ts
[warning] 5-5: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: document.body.innerHTML = ''
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation
(dom-content-modification)
[warning] 54-54: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: document.body.innerHTML = ''
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation
(dom-content-modification)
🔇 Additional comments (18)
package.json (1)
114-117: LGTM!src/.jest/setup.ts (1)
3-3: LGTM!src/jest-dom.d.ts (1)
1-22: LGTM!src/ui/test-utils.tsx (1)
1-55: LGTM!jest.config.js (1)
15-18: LGTM!src/logging/main/index.main.spec.ts (1)
130-133: LGTM!Also applies to: 147-155
src/screenSharing/main/ScreenSharingRequestTracker.main.spec.ts (1)
44-57: LGTM!src/screenSharing/main/desktopCapturerCache.main.spec.ts (1)
61-64: LGTM!src/ui/components/utils/createAnchor.spec.ts (1)
9-51: LGTM!Also applies to: 58-86
src/ui/components/utils/ReparentingContainer.spec.tsx (1)
1-33: LGTM!Also applies to: 41-159
docs/KNOWN_ISSUES.md (1)
22-41: LGTM!src/ui/components/Modal/ModalBackdrop.spec.tsx (1)
1-147: LGTM!src/ui/components/ClearCacheDialog/index.spec.tsx (1)
1-158: LGTM!src/ui/components/OutlookCredentialsDialog/index.spec.tsx (1)
1-194: LGTM!src/ui/components/ScreenSharingDialog/index.spec.tsx (1)
1-117: LGTM!src/ui/components/SupportedVersionDialog/index.spec.tsx (1)
1-167: LGTM!src/ui/components/UpdateDialog/index.spec.tsx (1)
1-102: LGTM!src/ui/components/AddServerView/index.spec.tsx (1)
154-157: No action needed — both enum members are valid and correctly used.The enum
ServerUrlResolutionStatusdefines bothINVALID_URLandINVALIDas valid members. The test case on line 156 correctly usesServerUrlResolutionStatus.INVALID, which is properly declared and consistent with its usage elsewhere in the codebase.
| it('clears the search filter when the clear-all button is clicked', async () => { | ||
| const user = userEvent.setup(); | ||
| renderWithStore(<DownloadsManagerView />, { | ||
| preloadedState: populatedState, | ||
| }); | ||
|
|
||
| const search = screen.getByLabelText( | ||
| 'downloads.filters.search' | ||
| ) as HTMLInputElement; | ||
| await user.type(search, 'beta'); | ||
| expect(search.value).toBe('beta'); | ||
|
|
||
| await user.click( | ||
| screen.getByRole('button', { name: 'downloads.filters.clear' }) | ||
| ); | ||
|
|
||
| expect(search.value).toBe(''); | ||
| expect(screen.getByTestId('download-item-1')).toBeInTheDocument(); | ||
| expect(screen.getByTestId('download-item-2')).toBeInTheDocument(); | ||
| }); |
There was a problem hiding this comment.
Make the clear-filter test deterministic by seeding initial filter state.
This case currently uses user.type even though this file documents typing/state sync instability in this renderer. Seed download-search before render, then clear it; that verifies the real persisted-filter path.
Proposed adjustment
it('clears the search filter when the clear-all button is clicked', async () => {
const user = userEvent.setup();
+ seedFilter('download-search', 'beta');
renderWithStore(<DownloadsManagerView />, {
preloadedState: populatedState,
});
const search = screen.getByLabelText(
'downloads.filters.search'
) as HTMLInputElement;
- await user.type(search, 'beta');
expect(search.value).toBe('beta');
await user.click(
screen.getByRole('button', { name: 'downloads.filters.clear' })
);🤖 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 `@src/ui/components/DownloadsManagerView/index.spec.tsx` around lines 152 -
171, The test 'clears the search filter when the clear-all button is clicked'
currently uses user.type to populate the search field, but this approach is
unstable in this renderer as documented. Instead, seed the download-search state
directly in the preloadedState object passed to renderWithStore with an initial
value of 'beta', then remove the user.type call entirely. This way the test
verifies the actual persisted-filter clearing behavior without relying on typing
interactions, making it deterministic and testing the real code path.
| afterEach(() => { | ||
| document.body.innerHTML = ''; | ||
| }); |
There was a problem hiding this comment.
Remove manual document.body.innerHTML cleanup; it can crash the suite.
These afterEach hooks can orphan portal anchors before RTL cleanup runs, triggering NotFoundError during unmount and aborting the run. Use component-level cleanup (unmount()/anchor-specific cleanup) instead of wiping document.body.
Suggested change
describe('createAnchor', () => {
- afterEach(() => {
- document.body.innerHTML = '';
- });
-
it('creates a div anchor by default and appends it to the body', () => {
const anchor = createAnchor('anchor-default');
@@
describe('deleteAnchor', () => {
- afterEach(() => {
- document.body.innerHTML = '';
- });
-
it('invokes the cleanup callback registered for the element', () => {Also applies to: 54-56
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 5-5: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: document.body.innerHTML = ''
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation
(dom-content-modification)
🤖 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 `@src/ui/components/utils/createAnchor.spec.ts` around lines 5 - 7, Remove the
manual document.body.innerHTML = '' cleanup from the afterEach hooks in
createAnchor.spec.ts, as this can orphan portal anchors and cause NotFoundError
crashes during RTL cleanup. Instead, rely on React Testing Library's automatic
cleanup by removing these afterEach blocks entirely, or use component-level
cleanup methods like unmount() where needed. This applies to multiple afterEach
hooks in the file (including the ones around lines 5-7 and 54-56).
| const Container = ReparentingContainer as unknown as React.FC< | ||
| React.HTMLAttributes<HTMLDivElement> & { children?: React.ReactNode } | ||
| >; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the contract mismatch and where test-side casts are used.
set -euo pipefail
fd -i 'ReparentingContainer.tsx' -x sed -n '1,140p' {}
echo '---'
rg -n -C3 'as unknown as React\\.FC|HTMLAttributes<HTMLDivElement>' src/ui/components/utils
echo '---'
rg -n -C3 'ReparentingContainer\\s*=\\s*forwardRef|type\\s+ReparentingContainerProps' src/ui/components/utilsRepository: RocketChat/Rocket.Chat.Electron
Length of output: 3778
Fix the component prop type instead of casting to React.FC in the test.
The component spreads extra props onto its inner div at runtime but declares only children in its prop type. Update ReparentingContainerProps to include React.HTMLAttributes<HTMLDivElement>, then remove the test-side cast on lines 37-39.
🤖 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 `@src/ui/components/utils/ReparentingContainer.spec.tsx` around lines 37 - 39,
The test file is using a type cast to work around a type mismatch in the
ReparentingContainer component. The root issue is that ReparentingContainerProps
does not declare that the component accepts HTML div attributes, even though it
spreads extra props onto its inner div element. Update the
ReparentingContainerProps type definition to extend
React.HTMLAttributes<HTMLDivElement> in addition to its existing properties,
then remove the unnecessary type cast on lines 37-39 where Container is defined
in the test file.
35bacdd to
c263950
Compare
Sets up component testing so the renderer layer can be covered. - Add devDeps: @testing-library/react 14.3.1, jest-dom 6.9.1, user-event 14.6.1, dom 9.3.4 (React 18 compatible) - Import @testing-library/jest-dom in src/.jest/setup.ts for DOM matchers - src/jest-dom.d.ts: register jest-dom matcher types for tsc (the dotted .jest dir is excluded from the TS program, so the augmentation lives in an included path) - src/ui/test-utils.tsx: renderWithStore() helper wrapping components in a redux Provider with preloaded state; re-exports RTL + userEvent; documents the react-i18next mock snippet for component specs - Example spec: ServersView/ErrorView.spec.tsx proves the pattern RTL works out-of-box under @kayahr/jest-electron-runner (real renderer DOM, no jsdom, no jest.config change). 807 tests pass, lint clean.
Adds RTL behavior tests for 14 renderer components — dialogs (Update, ScreenSharing, ClearCache, SupportedVersion, OutlookCredentials), containers (DownloadsManagerView, AddServerView, SideBar, Shell), leaf components (DownloadItem, ModalBackdrop, ServerButton) and the ui/utils helpers (createAnchor, ReparentingContainer). Also hardens 3 main-process specs (logging/index, ScreenSharingRequest- Tracker, desktopCapturerCache) with fake timers + afterEach cleanup — a real setInterval in logging/index.ts was leaking ref'd handles across isolateModules reloads and hanging the suite past --forceExit. Ratchets coverageThreshold to lines/statements 32, branches 28, functions 26. Coverage: lines 26.72% -> 33.29%, statements -> 33.65%, branches -> 29.33%, functions -> 27.31%. 912 tests pass, 2 skipped, 52 suites. Three component specs (ErrorView, ServerInfoContent, AboutDialog) are held back — they leak async/DOM teardown that the strict uncaught- exception handler in src/.jest/setup.ts turns into a process.exit(1); tracked in docs/KNOWN_ISSUES.md for a follow-up.
71b221c to
777936f
Compare
1 participant
What
Phase 3: introduces React Testing Library and uses it to test the renderer UI layer — the largest coverage gap. Stacked on Phase 2.
Coverage delta
912 tests pass, 2 skipped, 0 failures, 52 suites. Full
yarn test:coveragegreen.Infra (step 0)
@testing-library/react14.3.1,jest-dom6.9.1,user-event14.6.1,dom9.3.4 (React 18 compatible).@testing-library/jest-domimported insrc/.jest/setup.ts;src/jest-dom.d.tsregisters its matcher types for tsc (the dotted.jestdir is outside the TS program).src/ui/test-utils.tsx:renderWithStore()helper (redux Provider + preloaded state) + RTL/userEvent re-exports + the react-i18next mock snippet.@kayahr/jest-electron-runner(real renderer DOM, no jsdom, no jest.config project change).Component tests (14 specs)
Dialogs: UpdateDialog, ScreenSharingDialog, ClearCacheDialog, SupportedVersionDialog, OutlookCredentialsDialog. Containers: DownloadsManagerView, AddServerView, SideBar, Shell. Leaf: DownloadItem, ModalBackdrop, ServerButton. Helpers: createAnchor, ReparentingContainer.
Suite-stability fixes (the hard part)
Two distinct failure modes in the electron jest runner had to be solved before the suite could complete:
logging/index.ts'ssetInterval(flushErrorJsonl, 10_000)ran under real timers inindex.main.spec.ts, reloaded ~24× viaisolateModules, leaving ~20 live ref'd intervals that kept libuv alive past--forceExit. Fixed with fake timers +afterEachcleanup (also applied to ScreenSharingRequestTracker, desktopCapturerCache).afterEach(() => document.body.innerHTML = '')in two util specs raced RTL's auto-cleanup, so a portal's unmountremoveChildthrewNotFoundError→src/.jest/setup.ts's strictuncaughtExceptionhandler didprocess.exit(1), killing the whole run. Fixed by removing the redundant manual wipe.Key durable findings (in
docs/KNOWN_ISSUES.md): under this runner,userEvent.typedoes not commit to React controlled inputs — usefireEvent.change; and any leaked timer/async-rejection in one spec aborts the whole suite via the strict exit handler.Held back (follow-up)
Three specs (ErrorView, ServerInfoContent, AboutDialog) leak async/DOM teardown that the strict exit handler turns into a suite-killing
process.exit(1). Quarantined out of this PR and tracked indocs/KNOWN_ISSUES.md— they need either targeted async-cleanup or a test-aware exit handler (evaluated, deferred — that handler is shared infra with real CI-failure duty).Ratchet gate
coverageThresholdraised to lines/statements 32, branches 28, functions 26.Toward 50%
33% now. Remaining headroom: the held-back 3 specs once de-leaked, the rest of the dialogs/containers, and
ui/mainnon-webview logic. Each lands as a further wave; the goal is reachable viaui/without touching the webview/Electron-integration files.Summary by CodeRabbit
Documentation
Tests
Chores
Cumulative progress (full coverage initiative)
This PR is the 4th in a stacked series. Overall increase from the original baseline:
Tests: ~282 → 912 (+630).
Toward the 50% goal: +16 pts landed, ~17 to go — reachable via further
ui/waves (de-leaking the 3 quarantined specs + remaining dialogs/containers + non-webviewui/mainlogic), without touching webview/Electron-integration code.