test: raise full-surface line coverage to 70% with quick-win specs - #3429
test: raise full-surface line coverage to 70% with quick-win specs#3429jeanfbrito wants to merge 5 commits into
Conversation
Add and relocate unit/integration specs so Jest discovers them under the current testMatch rules, exercise main IPC and preloads under coverage, and extract small pure helpers for testability. Fixes ErrorView so the failed/reloading UI short-circuits correctly. Documents the new full-src coverage milestone in docs/COVERAGE.md (70.14% lines, 11876-line surface).
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR adds broad Jest and React Testing Library coverage across Electron services, preload bridges, server handling, UI components, desktop services, and utilities. It extracts log formatting and video-call URL validation, fixes ChangesCoverage expansion and behavior updates
Estimated code review effort: 5 (Critical) | ~90 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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: 5
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (17)
src/notifications/__tests__/attentionDrawing.spec.ts-42-42 (1)
42-42: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the conditional placeholder comment.
The comment states an action that the code does not perform. It gives no information about the setup. The coding guidelines ask for self-documenting code instead of unnecessary comments.
Based on the coding guideline "Avoid unnecessary comments; prefer self-documenting code through clear naming."
🤖 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/notifications/__tests__/attentionDrawing.spec.ts` at line 42, Remove the conditional placeholder comment near the attention-drawing test setup; do not add replacement commentary, leaving the surrounding setup and stopAttention behavior unchanged.Source: Coding guidelines
src/outlookCalendar/main/ipc.main.spec.ts-272-330 (1)
272-330: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the create, update, and delete calls named in the test title.
The test name states that the sync creates, updates, and deletes events. The only assertion checks
{ status: 'success' }. A regression that skips the delete ofrc-goneor the update ofrc-keepstill passes. Assert the request URLs and payloads foraxiosPostandaxiosDelete.🤖 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/outlookCalendar/main/ipc.main.spec.ts` around lines 272 - 330, Strengthen the get-events sync test by asserting the axiosPost calls for creating the new event and updating rc-keep, including their request URLs and payloads, and asserting the axiosDelete call removes rc-gone with the expected URL. Keep the existing success-result assertion and use the established request shapes from the sync implementation.src/documentViewer/main/ipc.spec.ts-1-19 (1)
1-19: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMain-process specs use the renderer
.spec.tssuffix in two files. Both suites exercise main-process modules and mock main-only APIs, but neither uses the*.main.spec.tssuffix that the coding guidelines require for main process tests. If Jest splits main and renderer into separate projects, both files run in the renderer project with the wrong test environment.
src/documentViewer/main/ipc.spec.ts#L1-L19: rename the file tosrc/documentViewer/main/ipc.main.spec.ts; it mockssession,webContents, and../../ipc/main.src/notifications/__tests__/attentionDrawing.spec.ts#L1-L27: rename the file tosrc/notifications/__tests__/attentionDrawing.main.spec.ts; it mocksapp.dockand../../ui/main/rootWindow.Based on the coding guideline "Use
*.main.spec.tsfor main process tests."🤖 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/documentViewer/main/ipc.spec.ts` around lines 1 - 19, Rename src/documentViewer/main/ipc.spec.ts to src/documentViewer/main/ipc.main.spec.ts and src/notifications/__tests__/attentionDrawing.spec.ts to src/notifications/__tests__/attentionDrawing.main.spec.ts so both main-process suites use the required *.main.spec.ts suffix; no test logic changes are needed.Source: Coding guidelines
src/outlookCalendar/main/ipc.main.spec.ts-231-237 (1)
231-237: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMatch the expected error message.
rejects.toThrow()with no argument passes on any thrown error, including aTypeErrorfrom an unrelated regression. Pass the expected message or a regular expression. The inline comment also records a known gap for the empty-credentials case. I can add that case with a per-testselectoverride if you want.🤖 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/outlookCalendar/main/ipc.main.spec.ts` around lines 231 - 237, Update the get-events rejection test for handlers.get('outlook-calendar/get-events') to assert the expected error message or a matching regular expression instead of accepting any thrown error. Keep the missing-server-path setup, and leave the documented empty-credentials coverage gap unchanged.src/outlookCalendar/reducers/__tests__/outlookReducers.spec.ts-39-44 (1)
39-44: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winStrengthen the null-override assertion.
The previous state passed here is already
null, sotoBeNull()passes whether the reducer applies the payload or returns the previous state. Pass a non-null previous state to make the assertion meaningful.🔧 Proposed fix
expect( - outlookCalendarSyncIntervalOverride(null, { + outlookCalendarSyncIntervalOverride(20, { type: APP_SETTINGS_LOADED, payload: { outlookCalendarSyncIntervalOverride: null }, } as any) ).toBeNull();🤖 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/outlookCalendar/reducers/__tests__/outlookReducers.spec.ts` around lines 39 - 44, Update the outlookCalendarSyncIntervalOverride test case for APP_SETTINGS_LOADED so its previous state is a non-null interval value while the payload override remains null, ensuring the assertion verifies the reducer applies the payload rather than returning the existing state.src/documentViewer/main/ipc.spec.ts-1-4 (1)
1-4: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the blank line inside the import group to fix the CI lint failure.
The
check (ubuntu-latest)job fails at line 4 with "There should be no empty line within import group". Move theelectronimport into the same group or remove the blank line as the linter expects.🔧 Proposed fix
-import { session, webContents } from 'electron'; - -import { SERVER_DOCUMENT_VIEWER_OPEN_URL } from '../../servers/actions'; -import { WEBVIEW_PDF_VIEWER_ATTACHED } from '../../ui/actions'; +import { session, webContents } from 'electron'; +import { SERVER_DOCUMENT_VIEWER_OPEN_URL } from '../../servers/actions'; +import { WEBVIEW_PDF_VIEWER_ATTACHED } from '../../ui/actions';🤖 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/documentViewer/main/ipc.spec.ts` around lines 1 - 4, Remove the blank line between the Electron import and the local imports in ipc.spec.ts, keeping all imports in a single contiguous group to satisfy the linter.Source: Linters/SAST tools
src/outlookCalendar/main/ipc.main.spec.ts-179-200 (1)
179-200: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd assertions to this test.
The test calls
outlook-calendar/set-user-tokenfour times with invalid payloads and asserts nothing. It passes whatever the handler does, including dispatching credentials for a mismatched user. Assert thatdispatchis not called and that no sync starts.🔧 Proposed fix
await handlers.get('outlook-calendar/set-user-token')?.( { id: 7 }, 'token', 'other-user' ); + await jest.advanceTimersByTimeAsync(200); + expect(dispatch).not.toHaveBeenCalled(); + expect(getOutlookEvents).not.toHaveBeenCalled();🤖 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/outlookCalendar/main/ipc.main.spec.ts` around lines 179 - 200, Update the set-user-token rejects invalid token payloads test to assert that dispatch is not called and no sync starts after all four invalid handler invocations. Use the existing dispatch and sync-related spies or mocks in the test setup, and preserve coverage of null token, null user, invalid ID, and mismatched-user payloads.src/outlookCalendar/main/ipc.main.spec.ts-393-403 (1)
393-403: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd assertions and remove the error-swallowing
try/catch.This test has no assertion beyond
watchFns.length. Thecatchblock discards any error thrown by the watch callback. The test therefore passes even when rescheduling fails. Let the callback throw, and assert the observable effect of rescheduling, for example thatgetOutlookEventsruns again after the new interval elapses.🔧 Proposed fix
- for (const fn of watchFns) { - try { - fn(30, 60); - } catch { - // ignore - } - } - await jest.advanceTimersByTimeAsync(11000); + for (const fn of watchFns) { + fn(30, 60); + } + await jest.advanceTimersByTimeAsync(11000); + expect(getOutlookEvents).toHaveBeenCalled();🤖 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/outlookCalendar/main/ipc.main.spec.ts` around lines 393 - 403, Update the “interval watch reschedules when value changes” test to invoke each watch callback directly without the error-swallowing try/catch, then assert the observable rescheduling behavior by verifying getOutlookEvents runs again after advancing timers beyond the new interval.src/screenSharing/__tests__/screenSharePicker.spec.tsx-184-207 (1)
184-207: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAssert the recovery behavior after the fetch failure.
The test only asserts that
invokewas called. It does not verify the behavior named in its title.fetchSourcescatches the error and keeps the previous source list. Assert that the dialog still renders and that no IPC response is sent while the picker stays open.🔧 Proposed fix
await waitFor(() => expect(invoke).toHaveBeenCalled()); + expect(screen.getByTestId('dialog')).toBeInTheDocument(); + expect(send).not.toHaveBeenCalled(); await act(async () => { setVisible?.(false); }); + expect(send).toHaveBeenCalledWith( + 'video-call-window/screen-sharing-source-responded', + null + );🤖 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/screenSharing/__tests__/screenSharePicker.spec.tsx` around lines 184 - 207, Strengthen the test around the ScreenSharePicker visibility flow so that after desktop-capturer-get-sources fails, the picker dialog remains rendered while visible and invoke has not sent an IPC response. Replace the call-only assertion with checks for the dialog’s continued presence and absence of a response, then retain the existing hide step.src/servers/main/preloadCoverage.spec.ts-1-5 (1)
1-5: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winName and path these server main specs consistently.
jest.config.jsonly picks upsrc/servers/main**/*.spec.tsandsrc/servers/main.spec.ts, not.main.spec.tsfiles, so renaming these to.main.spec.tsdoes not make them discoverable and may remove coverage from them.The main concern is keeping
src/servers/main.spec.tsdiscoverable: it is a main-process spec forsrc/servers/maincode, so rename or split it to a nested file undersrc/servers/main, such assrc/servers/main/convertToURL.spec.ts, or keep it where the Jest project already matches it.src/servers/main/preloadCoverage.spec.tsalready covers preload modules through the main/node project, so the main-process naming pattern does not apply there.🤖 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/servers/main/preloadCoverage.spec.ts` around lines 1 - 5, Keep src/servers/main/preloadCoverage.spec.ts unchanged because its existing name is already discoverable and it intentionally exercises preload modules through the main/node project. Rename or relocate src/servers/main.spec.ts into the matched src/servers/main/**/*.spec.ts pattern, such as a focused file alongside the covered implementation; ensure src/servers/main/resolveServerUrl.spec.ts and src/servers/main/setupServers.spec.ts retain discoverable *.spec.ts names and require no direct change.Sources: Coding guidelines, Path instructions
docs/COVERAGE.md-29-29 (1)
29-29: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse measurable coverage wording.
“Quick-win”, “large”, and “gotcha” are subjective terms. Replace them with terms that describe the scope or test constraint.
Suggested change
-| 2026-08-02 | Quick-win wave (`chore/test-coverage-quick-wins`) | 70.14% | 70.35% | 60.54% | 67.63% | 1842 | Full `src/**` collectCoverageFrom (no denominator gaming). Orphan specs nested for discovery; settings/UI/dialog RTL; main IPC (video call, Outlook, log viewer, notifications, downloads); preload coverage via main/node project under `--coverage`; ErrorView render short-circuit fix; pure helpers extracted (`validateVideoCallUrl`, `logFormatters`). | +| 2026-08-02 | Coverage expansion wave (`chore/test-coverage-quick-wins`) | 70.14% | 70.35% | 60.54% | 67.63% | 1842 | Full `src/**` collectCoverageFrom (no denominator gaming). Orphan specs nested for discovery; settings/UI/dialog RTL; main IPC (video call, Outlook, log viewer, notifications, downloads); preload coverage via main/node project under `--coverage`; ErrorView render short-circuit fix; pure helpers extracted (`validateVideoCallUrl`, `logFormatters`). | ... -- **Still large residual 0% / low modules:** +- **Residual 0% / low-coverage modules:** ... -- **Coverage gotcha:** +- **Coverage limitation:**As per coding guidelines, “Avoid subjective descriptors and use measurable descriptions.”
Also applies to: 34-35
🤖 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 `@docs/COVERAGE.md` at line 29, Update the 2026-08-02 coverage entry and the referenced lines 34–35 in docs/COVERAGE.md to remove subjective descriptors such as “Quick-win,” “large,” and “gotcha.” Replace them with objective wording that specifies the tested scope, coverage constraint, or measurable change while preserving the existing coverage data and technical details.Source: Coding guidelines
src/app/main/buildAssets.spec.ts-31-41 (1)
31-41: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFail when
buildAssetscannot load.Lines 33-39 accept every module-load error. Syntax errors, missing dependencies, and runtime failures therefore pass this test. Mock required native dependencies, then require the module without catching the error.
Suggested change
- try { - // eslint-disable-next-line `@typescript-eslint/no-var-requires` - require('../../buildAssets'); - } catch (error) { - // Missing native image tooling is acceptable; we still load what we can - expect(error).toBeDefined(); - } + // eslint-disable-next-line `@typescript-eslint/no-var-requires` + expect(require('../../buildAssets')).toBeDefined();🤖 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/app/main/buildAssets.spec.ts` around lines 31 - 41, Update the buildAssets module-load test to mock the required native image dependencies before importing buildAssets, then require it directly inside jest.isolateModules without catching or accepting errors. Keep the outer assertion that the isolated module load does not throw, so syntax, dependency, and runtime failures fail the test.src/logViewerWindow/__tests__/logViewerWindow.spec.tsx-100-103 (1)
100-103: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSample log fixture does not match the parser format used by the source.
LOG_LINE_REGEXinlogFormatters.tsexpects lines shaped like[timestamp] [level] message. The fixture'info: first message {...}'never matches this pattern, soparseLogLinessilently drops it and returns zero entries. The 'loads and renders log entries' and 'invokes actions when buttons are clicked' tests only assert oninvokecalls and button counts, so they still pass, but they do not actually verify that parsed log rows render. Use a fixture that matches the real format to give this suite real coverage of the render path.🧪 Proposed fixture fix
const sampleLog = [ - 'info: first message {"t":{"$date":"2026-01-01T00:00:00.000Z"}}', - 'error: boom {"t":{"$date":"2026-01-01T00:01:00.000Z"}}', + '[2026-01-01T00:00:00.000Z] [info] first message', + '[2026-01-01T00:01:00.000Z] [error] boom', ].join('\n');🤖 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/logViewerWindow/__tests__/logViewerWindow.spec.tsx` around lines 100 - 103, Update the sampleLog fixture used by the logViewerWindow tests to match the [timestamp] [level] message format required by LOG_LINE_REGEX and parseLogLines. Preserve the existing timestamp and level coverage while ensuring both lines parse into entries so the rendering and button-action assertions exercise real log rows.src/ui/components/FailureImage.spec.tsx-14-20 (1)
14-20: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd an assertion for the
st8color override.This test passes
st8='#000000'but only asserts theopacitystyle. Add an assertion that verifiesst8is actually applied (for example, checking the corresponding SVG element'sfillor inline style), otherwise the test does not cover what its name promises.🤖 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/FailureImage.spec.tsx` around lines 14 - 20, Extend the “accepts custom style and color overrides” test for FailureImage to assert that the st8="`#000000`" override is applied to the corresponding SVG element, such as through its fill or inline style, while preserving the existing opacity assertion.src/ui/components/TopBar/index.spec.tsx-26-32 (1)
26-32: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTest does not exercise the darwin-specific transparent branch.
sidebarBgbecomesundefinedonly whenprocess.platform === 'darwin' && isTransparentWindowEnabledperTopBar/index.tsx. This test setsisTransparentWindowEnabled: truebut never mocksprocess.platformto'darwin', so on a non-darwin test runner, the assertion passes without exercising that branch. Mockprocess.platformand assert onsidebarBg(e.g., via the renderedbgstyling) to genuinely cover this path.🤖 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/TopBar/index.spec.tsx` around lines 26 - 32, Update the “renders with transparent window enabled” test in the TopBar specs to mock process.platform as “darwin” while isTransparentWindowEnabled is true, then assert the rendered output reflects sidebarBg being undefined through its background styling. Restore the platform mock after the test to avoid affecting other cases.src/ui/components/utils/TooltipProvider.spec.tsx-74-85 (1)
74-85: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd an assertion to verify the tooltip actually closes.
This test performs the close interaction but has no
expect()call. It passes whether or notclose()removes the tooltip content, so it does not verify the behavior its title describes.🧪 Proposed fix
it('closes tooltip via context close', () => { render( <TooltipProvider> <Probe /> </TooltipProvider> ); fireEvent.click(screen.getByText('hover-me')); fireEvent.doubleClick(screen.getByText('hover-me')); act(() => { jest.advanceTimersByTime(200); }); + expect(screen.queryByTestId('tooltip-portal')).not.toBeInTheDocument(); });🤖 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/TooltipProvider.spec.tsx` around lines 74 - 85, Update the test “closes tooltip via context close” to assert that the tooltip content is no longer present after the click, double-click, and timer advancement. Add the expectation after the existing interaction sequence, using the rendered tooltip text or accessible query already exposed by Probe.src/ui/components/SettingsView/features/moreSettings.spec.tsx-142-169 (1)
142-169: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAssert on a stable disabled state instead of
className.
AvailableBrowsersrenders a disabled FuselageSelectwhen no browsers are available. Its disabled behavior is represented through accessibility props likedisabled, not a stable class-name contract. UsetoBeDisabled()or check the relateddisabledattribute/role behavior instead of matching/disabled/inclassName.🤖 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/SettingsView/features/moreSettings.spec.tsx` around lines 142 - 169, Update the AvailableBrowsers loading-placeholder test to assert the Select button’s disabled state via toBeDisabled() or its disabled attribute, rather than matching “disabled” in className; keep the enabled-browser test behavior unchanged.Source: Coding guidelines
🧹 Nitpick comments (20)
src/documentViewer/main/ipc.spec.ts (1)
164-171: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace the real-timer wait with fake timers.
The production handler calls
openExternalinsidesetTimeout(..., 10). The test waits 20 ms of wall-clock time. This adds flakiness under load. Usejest.useFakeTimers()and advance the timer instead.♻️ Proposed refactor
handler(event, 'https://external.example/doc'); expect(event.preventDefault).toHaveBeenCalled(); - await new Promise((resolve) => setTimeout(resolve, 20)); + await jest.advanceTimersByTimeAsync(20); expect(openExternal).toHaveBeenCalledWith('https://external.example/doc');Add
jest.useFakeTimers()inbeforeEachandjest.useRealTimers()inafterEach.🤖 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/documentViewer/main/ipc.spec.ts` around lines 164 - 171, Update the navigation handler test around openExternal to use Jest fake timers instead of a real 20 ms delay: enable fake timers in the test setup, advance them past the handler’s 10 ms timeout before asserting, and restore real timers during teardown.src/notifications/__tests__/attentionDrawing.spec.ts (1)
72-96: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestore
process.platformafter each test.Both tests override
process.platformand never restore it. The first test at lines 53-70 restores the value, but these two do not. The stub stays in place for every test that runs later in this file. Move the override intobeforeEach/afterEachso each test starts from a known platform.♻️ Proposed refactor
describe('attentionDrawing', () => { + const originalPlatform = process.platform; + + const setPlatform = (value: string) => { + Object.defineProperty(process, 'platform', { + value, + configurable: true, + }); + }; + + afterEach(() => { + setPlatform(originalPlatform); + });Then replace each inline
Object.defineProperty(process, 'platform', ...)call withsetPlatform('linux').🤖 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/notifications/__tests__/attentionDrawing.spec.ts` around lines 72 - 96, Ensure process.platform is isolated per test in the duplicate-notification and active-notification tests by moving platform setup into shared beforeEach/afterEach hooks that restore the original value. Replace the inline Object.defineProperty calls with the existing setPlatform('linux') helper, preserving each test’s behavior.src/notifications/main/setup.main.spec.ts (1)
101-112: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReset the module registry between tests.
src/notifications/main.tskeeps module-levelnotificationsandnotificationTypesmaps.beforeEachclearslistenersandnotificationInstances, but it does not clear those maps. Entries from earlier tests stay registered. A tag reused by two tests would then hit the "update existing notification" path instead of the create path, and the failure would be hard to diagnose. Addjest.resetModules()and re-require the module inbeforeEach.♻️ Proposed refactor
describe('notifications/main setupNotifications', () => { beforeEach(() => { jest.clearAllMocks(); + jest.resetModules(); listeners.clear(); notificationInstances.length = 0;Then load
setupNotificationsinsidebeforeEachwithrequire('../main')instead of the static import at line 99.🤖 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/notifications/main/setup.main.spec.ts` around lines 101 - 112, Reset Jest’s module registry in the setupNotifications test before each case, then load setupNotifications inside beforeEach via require('../main') instead of the static import. Keep the existing listener, instance, and mock resets, ensuring main.ts’s module-level notifications and notificationTypes maps are recreated for every test.src/videoCallWindow/__tests__/validateVideoCallUrl.spec.ts (1)
8-10: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
toBefor the http case.
new URL('http://localhost:8080/call').hrefequals the input string exactly.toContainweakens the assertion and differs from the https case above.♻️ Proposed change
- expect(validateVideoCallUrl('http://localhost:8080/call')).toContain( + expect(validateVideoCallUrl('http://localhost:8080/call')).toBe( 'http://localhost:8080/call' );🤖 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/videoCallWindow/__tests__/validateVideoCallUrl.spec.ts` around lines 8 - 10, Update the HTTP assertion in validateVideoCallUrl tests to use exact equality with toBe instead of substring matching with toContain, while leaving the expected URL value unchanged and preserving the HTTPS assertion behavior.src/videoCallWindow/validateVideoCallUrl.ts (1)
2-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider moving the protocol check outside the
tryblock.The protocol
Erroris thrown insidetryand then re-thrown by thecatch. The result is correct, but the flow is indirect. Parsing alone insidetrymakes the intent clear and prevents a futureTypeError-typed protocol error from being relabeled as a format error.♻️ Proposed refactor
export const validateVideoCallUrl = (url: string): string => { + let parsedUrl: URL; try { - const parsedUrl = new URL(url); - - const allowedProtocols = ['http:', 'https:']; - if (!allowedProtocols.includes(parsedUrl.protocol)) { - throw new Error( - `Invalid URL protocol: ${parsedUrl.protocol}. Only http: and https: are allowed.` - ); - } - - return parsedUrl.href; - } catch (error) { - if (error instanceof TypeError) { - throw new Error(`Invalid URL format: ${url}`); - } - throw error; - } + parsedUrl = new URL(url); + } catch { + throw new Error(`Invalid URL format: ${url}`); + } + + const allowedProtocols = ['http:', 'https:']; + if (!allowedProtocols.includes(parsedUrl.protocol)) { + throw new Error( + `Invalid URL protocol: ${parsedUrl.protocol}. Only http: and https: are allowed.` + ); + } + + return parsedUrl.href; };🤖 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/videoCallWindow/validateVideoCallUrl.ts` around lines 2 - 18, Update the URL validation flow so only the `new URL(url)` parsing in `validateVideoCallUrl` is inside the `try`/`catch`; perform the allowed-protocol check after parsing, preserving the existing protocol error and malformed-URL handling.src/videoCallWindow/main/ipc.main.spec.ts (1)
206-244: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftSeveral tests execute IPC handlers without verifying behavior. The shared root cause is that the replacement suite calls handlers to raise line coverage but omits assertions on their observable effects, so these tests pass even if the handlers regress.
src/videoCallWindow/main/ipc.main.spec.ts#L206-L244: remove the blankettry/catchand assert a concrete outcome per channel, such as the return type ofvideo-call-window/get-languageand a call toprewarmDesktopCapturerCache.src/videoCallWindow/main/ipc.main.spec.ts#L267-L288: assert the fallback argument passed togetWebContentsByServerUrl, and add an assertion to the no-op test.src/videoCallWindow/main/ipc.main.spec.ts#L290-L306: assert the value thatget-provider-syncwrites toevent.returnValue.🤖 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/videoCallWindow/main/ipc.main.spec.ts` around lines 206 - 244, Replace coverage-only handler invocations with behavioral assertions: in src/videoCallWindow/main/ipc.main.spec.ts lines 206-244, remove blanket try/catch handling and assert concrete results for each channel, including get-language’s return type and prewarmDesktopCapturerCache invocation; in lines 267-288, assert the fallback passed to getWebContentsByServerUrl and add an assertion for the no-op test; in lines 290-306, assert the value written to event.returnValue by get-provider-sync.src/servers/main/preloadCoverage.spec.ts (3)
229-231: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace the repeated eslint-disable comments with one file-level directive.
The
// eslint-disable-next-line@typescript-eslint/no-var-requires`` comment repeats about 20 times in this file. Put/* eslint-disable@typescript-eslint/no-var-requires` */` at the top of the file instead. The `require` calls must stay inside the tests because `jest.resetModules()` runs in `beforeEach`.🤖 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/servers/main/preloadCoverage.spec.ts` around lines 229 - 231, Replace the repeated per-line `@typescript-eslint/no-var-requires` disable comments in preloadCoverage.spec.ts with a single file-level /* eslint-disable `@typescript-eslint/no-var-requires` */ directive at the top, while keeping all require calls inside the tests so jest.resetModules() continues to work.
482-564: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSplit this test into one test per preload module.
This single test exercises the Outlook calendar preload, user presence, message box, sidebar, and screen sharing. A failure in the first block hides the later blocks, and the test name does not identify the failing module. Split it into five tests.
The fake-timer block adds a second risk. Line 506 calls
jest.useFakeTimers()and line 530 callsjest.useRealTimers(). If any assertion between those lines throws, fake timers leak into later tests. Move the restore intoafterEach.🤖 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/servers/main/preloadCoverage.spec.ts` around lines 482 - 564, Split the combined test into five independently named tests covering Outlook, user presence, message box, sidebar, and screen sharing preloads, keeping each module’s existing setup and assertions together. Move fake-timer cleanup for the user presence test into an afterEach hook so jest.useRealTimers() runs even when that test fails, and remove the inline restoration.
412-451: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis loop asserts no behavior.
The loop invokes every registered
listenhandler, swallows all errors, and then asserts only thatrequestandlistenwere called. The assertions pass even if every handler throws.listenToNotificationsRequestshas a testable contract: aDOWNLOADScategory dispatchesSIDE_BAR_DOWNLOADS_BUTTON_CLICKED, and any other category dispatchesWEBVIEW_FOCUS_REQUESTEDwith the server URL (seesrc/notifications/preload.tslines 96-150).Select the handlers by their action type and assert the resulting
dispatchpayloads.🤖 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/servers/main/preloadCoverage.spec.ts` around lines 412 - 451, Replace the broad handler-invocation loop in the listenToNotificationsRequests test with targeted handlers selected by their action type. Invoke the relevant handlers with DOWNLOADS and non-DOWNLOADS payloads, then assert dispatch receives SIDE_BAR_DOWNLOADS_BUTTON_CLICKED for DOWNLOADS and WEBVIEW_FOCUS_REQUESTED including the server URL for other categories; do not swallow handler errors or rely only on request/listen call assertions.src/servers/__tests__/fetchInfo.spec.ts (1)
11-28: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert the exact resolved URL.
fetchInforeturnsnew URL('..', apiInfoResponse.url).href, which ishttps://open.rocket.chat/for this mock. AtoContaincheck passes even if the path stripping breaks. Assert the exact value to protect that behavior.♻️ Proposed assertion tightening
const [url, version] = await fetchInfo('https://open.rocket.chat'); expect(version).toBe('6.5.0'); - expect(url).toContain('open.rocket.chat'); + expect(url).toBe('https://open.rocket.chat/'); expect(global.fetch).toHaveBeenCalledTimes(2);🤖 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/servers/__tests__/fetchInfo.spec.ts` around lines 11 - 28, Update the URL assertion in the fetchInfo test to compare the resolved URL against the exact expected value, including the trailing slash, instead of using a substring check. Keep the existing version and fetch-call assertions unchanged.src/servers/main/setupServers.spec.ts (2)
153-173: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the storage clearing, not only the lookup.
The test name states that the handler clears guest storage, but the assertion only checks that
getWebContentsByServerUrlwas called. Capture the mockedsessionobject and assertclearStorageData,clearCache, andreloadwere called.🤖 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/servers/main/setupServers.spec.ts` around lines 153 - 173, Update the test around setupServers and the listenHandlers loop to capture the mocked session object, then assert that clearStorageData, clearCache, and reload are each called. Replace or supplement the getWebContentsByServerUrl assertion so the test verifies the guest-storage clearing behavior described by the test name.
127-151: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis test cannot fail.
Line 150 asserts
handler || listenHandlers.size, which is truthy whenever any listener is registered. Thehandlerlookup at line 132 matches the key against the literal'SERVER_URL_RESOLUTION', butlistenreceives the action-type value, not the constant name, sohandleris likelyundefined. The loop also swallows every error.Register the handler by importing
SERVER_URL_RESOLUTION_REQUESTEDfrom the actions module, invoke that handler directly, and assert the dispatchedSERVER_URL_RESOLVEDaction carriesmeta.id === '1'andmeta.response === true.🤖 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/servers/main/setupServers.spec.ts` around lines 127 - 151, Make the SERVER_URL_RESOLUTION_REQUESTED test deterministic by importing the action constant, locating its registered handler directly, and invoking it with the test payload and metadata. Remove the broad listener iteration and swallowed errors, then assert the resulting SERVER_URL_RESOLVED dispatch has meta.id equal to '1' and meta.response equal to true.src/servers/main/resolveServerUrl.spec.ts (1)
58-70: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a case for the subdomain retry branch.
resolveServerUrlretries withurls.rocketchat.subdomain(input)when the input has no protocol, no dot, and is notlocalhost(seesrc/servers/main.tslines 66-111). Every current failure case uses an absolute URL, so the recursion branch stays untested. Add a case such asresolveServerUrl('myworkspace')with a rejectinginvoke.🤖 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/servers/main/resolveServerUrl.spec.ts` around lines 58 - 70, Add a test in the resolveServerUrl suite covering a protocol-less, dotless, non-localhost input such as “myworkspace”; configure invoke to reject and assert the expected invalid resolution status after the subdomain retry path runs.src/servers/main.spec.ts (1)
34-60: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate
convertToURLcoverage.
src/servers/main/resolveServerUrl.spec.tsalso importsconvertToURLand asserts bare-hostname handling (line 72-74 of that file), and it repeats the same six mock blocks. Keep theconvertToURLcases in one spec to avoid two mock setups drifting apart.🤖 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/servers/main.spec.ts` around lines 34 - 60, Consolidate the duplicate convertToURL coverage by keeping the URL parsing cases in a single spec, preferably the existing tests in resolveServerUrl.spec.ts. Remove the overlapping convertToURL tests and their associated mock blocks from the main.spec.ts describe block, while preserving any tests specific to other behavior.src/ui/main/menuBar.spec.ts (2)
13-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename main-process specs to
*.main.spec.ts. All three files test main-process modules (they mockelectronmain APIs such asapp,Menu,BrowserWindow,app.dock, andelectron-updater), but use the renderer-oriented*.spec.tssuffix instead of the project's*.main.spec.tsconvention already followed byrootWindowGeometry.main.spec.ts,touchBar.main.spec.ts,trayIcon.main.spec.ts, andsetup.main.spec.tsin this same cohort.
src/ui/main/menuBar.spec.ts#L13-L19: rename tomenuBar.main.spec.ts.src/ui/main/dock.spec.ts#L1-L8: rename todock.main.spec.ts.src/updates/main/setupUpdates.spec.ts#L29-L42: rename tosetupUpdates.main.spec.ts.As per coding guidelines, "
**/*.main.spec.ts: Main-process specs use*.main.spec.ts." and "**/*.spec.ts: Use*.spec.tsfor renderer process tests."🤖 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/main/menuBar.spec.ts` around lines 13 - 19, Rename the main-process spec files to follow the project convention: src/ui/main/menuBar.spec.ts to menuBar.main.spec.ts, src/ui/main/dock.spec.ts to dock.main.spec.ts, and src/updates/main/setupUpdates.spec.ts to setupUpdates.main.spec.ts. Preserve each test’s contents and behavior.Source: Coding guidelines
356-406: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDo not silently swallow click-handler exceptions.
Both loops catch every exception from
item.click?.(...)and discard it. The first test's name states it invokes handlers "without throwing," but a thrown exception from a real regression is caught and ignored, so the test passes regardless. Collect caught errors into an array and assert the array is empty (or a known allow-list of expected platform-only errors), so a genuine regression fails the test.♻️ Proposed fix to surface unexpected click-handler failures
const template = selectMenuBarTemplate(state) as MenuItemConstructorOptions[]; const clickables = collectClickableItems(template); + const errors: unknown[] = []; for (const item of clickables) { try { await Promise.resolve( item.click?.({} as any, mockBrowserWindow as any, {} as any) ); } catch { - // Some handlers reach optional platform APIs; exercise still counts. + errors.push(item.id); } } + expect(errors).toEqual([]); expect(dispatch).toHaveBeenCalled(); expect(getRootWindow).toHaveBeenCalled();🤖 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/main/menuBar.spec.ts` around lines 356 - 406, Update both click-handler loops in the tests “invokes click handlers without throwing for common menu actions” and “builds selectAppMenuPopupTemplate and runs its click handlers” to collect caught exceptions instead of discarding them, then assert that no unexpected errors were captured; if platform-only failures are intentionally allowed, filter them through an explicit allow-list while ensuring genuine handler regressions fail the tests.src/ui/main/dock.spec.ts (1)
21-37: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a darwin-path test to cover
DockService.initialize()core logic.Both tests here either skip
setUp()entirely or call it on a non-darwin platform, which only exercises the early-return guard. The badge/bounce watch registrations ininitialize()(seesrc/ui/main/dock.ts:6-33) are never invoked. Mock'../../store'(as done intouchBar.main.spec.tsandtrayIcon.main.spec.ts), setprocess.platformto'darwin', callsetUp(), then trigger the watch callbacks and assertapp.dock.setBadgeandapp.dock.bounceare called as expected.🤖 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/main/dock.spec.ts` around lines 21 - 37, Add a Darwin-specific test in the dock service spec that mocks ../../store, sets process.platform to darwin, calls DockService.setUp(), invokes the registered badge and bounce watch callbacks, and asserts app.dock.setBadge and app.dock.bounce receive the expected values. Keep the existing export and non-Darwin no-op tests unchanged.src/ui/components/ServersView/PdfContent.spec.tsx (1)
22-43: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winStrengthen weak assertions that don't verify the behavior under test.
Both tests assert on values unrelated to the actual PDF rendering:
expect(container.firstChild).toBeTruthy()only confirms something rendered, not that awebviewelement with the correctsrcmounted.expect(screen.queryByRole('progressbar')).toBeFalsy()checks aprogressbarrole, not that thewebviewwas removed after the URL became empty.As written, these tests would still pass even if
PdfContentrendered the wrong URL or failed to clear the previous document. Query thewebviewelement directly and assert on itssrcattribute and its removal.🧪 Suggested stronger assertions
it('renders webview for a pdf url after delay', () => { const { container } = render( <PdfContent url='file:///doc.pdf' partition='persist:server' /> ); act(() => { jest.advanceTimersByTime(150); }); - // webview may be custom element; ensure component mounted - expect(container.firstChild).toBeTruthy(); + const webview = container.querySelector('webview'); + expect(webview).toBeTruthy(); + expect(webview?.getAttribute('src')).toBe('file:///doc.pdf'); }); it('clears document when url becomes empty', () => { const { rerender, container } = render( <PdfContent url='file:///doc.pdf' partition='persist:server' /> ); act(() => { jest.advanceTimersByTime(150); }); rerender(<PdfContent url='' partition='persist:server' />); - expect(screen.queryByRole('progressbar')).toBeFalsy(); + expect(container.querySelector('webview')).toBeNull(); });🤖 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/ServersView/PdfContent.spec.tsx` around lines 22 - 43, Strengthen the assertions in the `PdfContent` tests: after advancing timers, query the rendered `webview` element and verify its `src` attribute matches the PDF URL, then after rerendering with an empty URL, assert that the `webview` is removed. Replace the unrelated `container.firstChild` and `progressbar` checks while preserving the existing timing and rerender flow.src/ui/components/ServersView/ServerPane.spec.tsx (1)
71-90: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the specific dispatched action, not just that
dispatchwas called.
expect(spy).toHaveBeenCalled()(Line 89) passes even if the wrong action is dispatched on reload. Assert the actual action type/payload to catch regressions in the reload behavior.🤖 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/ServersView/ServerPane.spec.tsx` around lines 71 - 90, Update the “shows error view when failed and reloads” test to assert the specific action dispatched by the error-reload interaction, including its expected type and payload, rather than only verifying that dispatch was called. Use the existing ServerPane reload behavior and store action definitions to form the exact expectation.src/ui/components/SettingsView/features/moreSettings.spec.tsx (1)
158-169: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest the actual selection-dispatch behavior instead of asserting an import exists.
Line 168 only checks that
SETTINGS_SELECTED_BROWSER_CHANGEDis defined. This does not verify that selecting a browser inAvailableBrowsersdispatches this action with the correct payload. Given this PR's goal of raising meaningful coverage, replace this assertion with an interaction test that selects an option and asserts the dispatched action, similar to the pattern used insettingsToggles.spec.tsx.♻️ Proposed direction
- // Keep import used for type-level linkage to action constant - expect(SETTINGS_SELECTED_BROWSER_CHANGED).toBeDefined(); + fireEvent.click(screen.getByRole('button')); + fireEvent.click(screen.getByText('Chrome')); + expect(dispatchSpy).toHaveBeenCalledWith({ + type: SETTINGS_SELECTED_BROWSER_CHANGED, + payload: 'Chrome', + });As per coding guidelines, "Avoid unnecessary comments; prefer self-documenting code through clear naming."
🤖 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/SettingsView/features/moreSettings.spec.tsx` around lines 158 - 169, Replace the SETTINGS_SELECTED_BROWSER_CHANGED definedness assertion in the enables select when browsers are available test with an interaction test: select a browser option in AvailableBrowsers and assert the dispatched action contains the expected selected-browser payload, following the dispatch-mocking pattern used by settingsToggles.spec.tsx. Remove the now-unnecessary import-only comment and preserve the existing availability and title assertions.Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 10ced613-1a92-4a3d-8fe6-19d75d8d64f2
📒 Files selected for processing (70)
docs/COVERAGE.mdsrc/app/main/buildAssets.spec.tssrc/app/main/mainEntry.spec.tssrc/documentViewer/main/ipc.spec.tssrc/downloads/__tests__/actions.spec.tssrc/downloads/__tests__/notifications.spec.tssrc/downloads/main/integration.spec.tssrc/i18n/__tests__/renderer.spec.tssrc/i18n/__tests__/resources.spec.tssrc/logViewerWindow/__tests__/LogEntry.spec.tsxsrc/logViewerWindow/__tests__/logFormatters.spec.tssrc/logViewerWindow/__tests__/logViewerWindow.spec.tsxsrc/logViewerWindow/logFormatters.tssrc/logViewerWindow/logViewerWindow.tsxsrc/logViewerWindow/main/ipc.main.spec.tssrc/logging/__tests__/cleanup.spec.tssrc/logging/__tests__/fallback.spec.tssrc/logging/__tests__/scopes.spec.tssrc/logging/__tests__/utils.spec.tssrc/notifications/__tests__/attentionDrawing.spec.tssrc/notifications/main/setup.main.spec.tssrc/outlookCalendar/main/ipc.main.spec.tssrc/outlookCalendar/reducers/__tests__/outlookReducers.spec.tssrc/screenSharing/__tests__/resolveStandaloneOriginWindow.main.spec.tssrc/screenSharing/__tests__/screenSharePicker.spec.tsxsrc/servers/__tests__/common.spec.tssrc/servers/__tests__/fetchInfo.spec.tssrc/servers/main.spec.tssrc/servers/main/preloadCoverage.spec.tssrc/servers/main/resolveServerUrl.spec.tssrc/servers/main/setupServers.spec.tssrc/ui/components/AboutDialog/index.spec.tsxsrc/ui/components/App.spec.tsxsrc/ui/components/CertificatesManager/CertificatesManager.spec.tsxsrc/ui/components/FailureImage.spec.tsxsrc/ui/components/SelectClientCertificateDialog/index.spec.tsxsrc/ui/components/ServerInfoContent.spec.tsxsrc/ui/components/ServerInfoModal/index.spec.tsxsrc/ui/components/ServersView/DocumentViewer.spec.tsxsrc/ui/components/ServersView/ErrorView.spec.tsxsrc/ui/components/ServersView/ErrorView.tsxsrc/ui/components/ServersView/MarkdownContent.spec.tsxsrc/ui/components/ServersView/PdfContent.spec.tsxsrc/ui/components/ServersView/ServerPane.spec.tsxsrc/ui/components/SettingsView/SettingsView.spec.tsxsrc/ui/components/SettingsView/features/ClearPermittedScreenCaptureServers.spec.tsxsrc/ui/components/SettingsView/features/MenuBar.spec.tsxsrc/ui/components/SettingsView/features/MinimizeOnClose.spec.tsxsrc/ui/components/SettingsView/features/ToggleField.spec.tsxsrc/ui/components/SettingsView/features/moreSettings.spec.tsxsrc/ui/components/SettingsView/features/settingsToggles.spec.tsxsrc/ui/components/SettingsView/tabs.spec.tsxsrc/ui/components/TopBar/index.spec.tsxsrc/ui/components/utils/ErrorCatcher.spec.tsxsrc/ui/components/utils/TooltipProvider.spec.tsxsrc/ui/components/utils/createAnchor.renderer.spec.tssrc/ui/components/utils/getServerDomId.spec.tssrc/ui/components/utils/getServerInitials.spec.tssrc/ui/main/__tests__/rootWindowGeometry.main.spec.tssrc/ui/main/dock.spec.tssrc/ui/main/menuBar.spec.tssrc/ui/main/touchBar.main.spec.tssrc/ui/main/trayIcon.main.spec.tssrc/updates/main/setupUpdates.spec.tssrc/userPresence/main/setup.main.spec.tssrc/videoCallWindow/__tests__/screenSharePickerMount.spec.tssrc/videoCallWindow/__tests__/validateVideoCallUrl.spec.tssrc/videoCallWindow/main/ipc.main.spec.tssrc/videoCallWindow/validateVideoCallUrl.tssrc/videoCallWindow/video-call-window.ts
| import fs from 'fs'; | ||
| import path from 'path'; | ||
|
|
||
| /** | ||
| * buildAssets.ts is a CLI-style asset builder. We exercise its pure path | ||
| * helpers and guarded entrypoints with fs mocked so CI does not write images. | ||
| */ | ||
|
|
||
| jest.mock('fs', () => { | ||
| const actual = jest.requireActual('fs'); | ||
| return { | ||
| ...actual, | ||
| existsSync: jest.fn(() => true), | ||
| mkdirSync: jest.fn(), | ||
| writeFileSync: jest.fn(), | ||
| readFileSync: jest.fn(() => Buffer.from('fake')), | ||
| promises: { | ||
| ...actual.promises, | ||
| mkdir: jest.fn(async () => undefined), | ||
| writeFile: jest.fn(async () => undefined), | ||
| readFile: jest.fn(async () => Buffer.from('fake')), | ||
| }, | ||
| }; | ||
| }); | ||
|
|
||
| describe('buildAssets module load', () => { | ||
| it('is a TypeScript module that can be required under mocks', () => { | ||
| // Avoid executing the CLI main by not invoking default export if present. | ||
| // Importing for coverage of top-level constants/helpers when the module | ||
| // is structured that way; if it self-runs, the fs mocks keep it safe. | ||
| expect(() => { | ||
| jest.isolateModules(() => { | ||
| try { | ||
| // eslint-disable-next-line @typescript-eslint/no-var-requires | ||
| require('../../buildAssets'); | ||
| } catch (error) { | ||
| // Missing native image tooling is acceptable; we still load what we can | ||
| expect(error).toBeDefined(); | ||
| } | ||
| }); | ||
| }).not.toThrow(); | ||
| expect(path.join('a', 'b')).toContain('a'); | ||
| expect(fs.existsSync).toBeDefined(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Rename main-process specs with the required suffix.
These main-process tests use .spec.ts instead of .main.spec.ts. This can select the wrong Jest project and prevents the required main-process test naming convention.
src/app/main/buildAssets.spec.ts#L1-L45: rename tosrc/app/main/buildAssets.main.spec.ts.src/app/main/mainEntry.spec.ts#L1-L34: rename tosrc/app/main/mainEntry.main.spec.ts.src/downloads/main/integration.spec.ts#L4-L52: rename tosrc/downloads/main/integration.main.spec.ts.
As per coding guidelines, “Main-process specs use *.main.spec.ts.”
📍 Affects 3 files
src/app/main/buildAssets.spec.ts#L1-L45(this comment)src/app/main/mainEntry.spec.ts#L1-L34src/downloads/main/integration.spec.ts#L4-L52
🤖 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/app/main/buildAssets.spec.ts` around lines 1 - 45, Rename the
main-process spec files to use the required *.main.spec.ts suffix:
src/app/main/buildAssets.spec.ts lines 1-45 to
src/app/main/buildAssets.main.spec.ts, src/app/main/mainEntry.spec.ts lines 1-34
to src/app/main/mainEntry.main.spec.ts, and
src/downloads/main/integration.spec.ts lines 4-52 to
src/downloads/main/integration.main.spec.ts; no test-content changes are needed.
Source: Coding guidelines
The video-call ipc.main.spec.ts rewrite in the previous commit dropped popup URL-scheme denial, shared-vs-isolated session permission-handler wiring, session-partition resolution, restore idempotency, and same-conference reopen coverage with nothing replacing it. Restores the richer mocking harness and those assertions, merged with the newer lifecycle-channel coverage. Also replaces a coverage-padding test in preloadCoverage.spec.ts that only asserted dispatch-was-called with real per-call assertions on the actual dispatched action types and payloads.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/videoCallWindow/main/ipc.main.spec.ts (1)
960-972: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winStrengthen the
get-provider-syncassertion beyond “does not throw”.This test only checks that the listener does not throw. Invoke the listener with a prepared event and assert the
event.returnValuecontains the expected provider value.
🤖 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/servers/main/preloadCoverage.spec.ts`:
- Around line 6-14: Rename preloadCoverage.spec.ts to
preloadCoverage.main.spec.ts so the main-process harness follows the required
*.main.spec.ts naming convention, preserving its contents and behavior.
- Around line 347-357: Move the `@typescript-eslint/no-var-requires` suppression
directives in the preloadCoverage spec so each directly precedes its
corresponding require() call for getE2ePdfPreviewSizeLimit and for
openDocumentViewer/supportedDocumentViewerFormats. Keep the existing imports and
assertions unchanged while ensuring both reported require lines are suppressed.
---
Nitpick comments:
In `@src/videoCallWindow/main/ipc.main.spec.ts`:
- Around line 960-972: Update the get-provider-sync test around the
ipcMainOnHandlers listener to invoke it with the prepared event and assert that
event.returnValue equals the expected provider value, replacing the current
not.toThrow-only assertion while retaining the listener existence check.
🪄 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 Plus
Run ID: a7512967-114f-4042-bcc7-a83458700ad7
📒 Files selected for processing (2)
src/servers/main/preloadCoverage.spec.tssrc/videoCallWindow/main/ipc.main.spec.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: check (ubuntu-latest)
- GitHub Check: check (windows-latest)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript for new code unless explicitly told otherwise.
Use Fuselage components from@rocket.chat/fuselagefor UI work unless the design requires something Fuselage does not provide.
CheckTheme.d.tsfor valid color tokens before using Fuselage colors.
Verify library props, APIs, and tokens against official docs or local.d.tsfiles instead of assuming.
Use React functional components with hooks.
Redux actions follow FSA shape.
Use camelCase for file names and PascalCase for components.
Prefer clear names over unnecessary comments.
Prefer editing existing files over creating new abstractions unless the new abstraction removes real complexity or matches an existing pattern.
**/*.{ts,tsx}: Use TypeScript for all new code unless explicitly told otherwise.
Use Fuselage components for all UI work; create custom components only when Fuselage lacks the required functionality.
Import Fuselage components from@rocket.chat/fuselage.
Use only valid color tokens documented byTheme.d.ts.
Use optional chaining with fallbacks for platform-specific APIs, especially Linux-only process APIs such asprocess.getuid(),getgid(),geteuid(), andgetegid().
Use TypeScript strict mode.
Redux actions must follow the Flux Standard Action pattern.
Use camelCase for file names and PascalCase for component names.
Avoid unnecessary comments; prefer self-documenting code through clear naming.
Do not commit or push without explicit user permission.
Verify library APIs, props, tokens, and types against official documentation and.d.tsfiles instead of assuming they are valid.
Files:
src/videoCallWindow/main/ipc.main.spec.tssrc/servers/main/preloadCoverage.spec.ts
**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Renderer specs use
*.spec.ts/*.spec.tsx.
Files:
src/videoCallWindow/main/ipc.main.spec.tssrc/servers/main/preloadCoverage.spec.ts
**/*.main.spec.ts
📄 CodeRabbit inference engine (AGENTS.md)
Main-process specs use
*.main.spec.ts.Use
*.main.spec.tsfor main process tests.
Files:
src/videoCallWindow/main/ipc.main.spec.ts
src/*/*/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Renderer specs must live in a Jest-matched nested path, such as
src/<module>/<subdir>/*.spec.ts(x); flatsrc/<module>/*.spec.tsfiles are not discovered by the currenttestMatch.
Files:
src/videoCallWindow/main/ipc.main.spec.tssrc/servers/main/preloadCoverage.spec.ts
**/*.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
*.spec.tsfor renderer process tests.
Files:
src/videoCallWindow/main/ipc.main.spec.tssrc/servers/main/preloadCoverage.spec.ts
src/**/*.{spec.ts,spec.tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Renderer test files should be placed in nested module paths such as
src/<module>/<subdir>/*.spec.ts(x)so Jest discovers them.
Files:
src/videoCallWindow/main/ipc.main.spec.tssrc/servers/main/preloadCoverage.spec.ts
🪛 ESLint
src/servers/main/preloadCoverage.spec.ts
[error] 350-350: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
[error] 357-357: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
🔇 Additional comments (5)
src/videoCallWindow/main/ipc.main.spec.ts (5)
2-21: LGTM!Also applies to: 37-43
71-78: LGTM!Also applies to: 174-175, 184-187, 214-217
279-300: LGTM!
349-356: LGTM!
378-486: LGTM!Also applies to: 906-912
| import { | ||
| WEBVIEW_UNREAD_CHANGED, | ||
| WEBVIEW_SERVER_VERSION_UPDATED, | ||
| WEBVIEW_SERVER_UNIQUE_ID_UPDATED, | ||
| WEBVIEW_TITLE_CHANGED, | ||
| WEBVIEW_GIT_COMMIT_HASH_CHECK, | ||
| WEBVIEW_FORCE_RELOAD_WITH_CACHE_CLEAR, | ||
| WEBVIEW_USER_LOGGED_IN, | ||
| } from '../../ui/actions'; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use the main-process test suffix.
src/servers/main/preloadCoverage.spec.ts is a main/node harness test, but it uses the renderer-spec suffix. Rename it to src/servers/main/preloadCoverage.main.spec.ts.
As per coding guidelines, “Main-process specs use *.main.spec.ts.”
🤖 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/servers/main/preloadCoverage.spec.ts` around lines 6 - 14, Rename
preloadCoverage.spec.ts to preloadCoverage.main.spec.ts so the main-process
harness follows the required *.main.spec.ts naming convention, preserving its
contents and behavior.
Source: Coding guidelines
Fixes all 5 actionable comments plus the 17 minor and 20 nitpick findings CodeRabbit left on PR #3429: - Renamed main-process specs to the *.main.spec.ts convention (buildAssets, mainEntry, downloads/integration, menuBar, dock) so Jest routes them to the correct project. - Fixed import/first and other lint failures blocking CI (i18n renderer, logViewerWindow, documentViewer, PdfContent). - Replaced weak/no-op assertions across outlookCalendar, setupServers, preloadCoverage, screenSharePicker, FailureImage, TopBar, TooltipProvider, moreSettings, menuBar, dock, PdfContent, and ServerPane specs with real assertions on dispatched actions, DOM state, or IPC payloads so a genuine regression would fail the test. - Fixed a stale fs-mock reference in setupServers.spec.ts that left the app-server fallback branch fully unexercised. - Split an overloaded preload-coverage test into 5 focused tests and consolidated 28 repeated eslint-disable comments into one file-level directive. - Restored security-relevant coverage in videoCallWindow ipc.main.spec.ts (popup scheme denial, session permission wiring) that had been dropped in the original rewrite, plus a real get-provider-sync assertion.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/downloads/main/integration.main.spec.ts (2)
288-323: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRestore
Date.nowon every test exit.The test overwrites global
Date.nowand restores it only at Line 323. If a handler or assertion throws first, later tests use the fake timestamp. Wrap the override and test body intry/finally.🤖 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/downloads/main/integration.main.spec.ts` around lines 288 - 323, Update the test case around Date.now and the pause/resume/cancel handler assertions so the original Date.now reference is restored in a finally block, regardless of whether setup, handlers, or assertions throw. Keep the existing test flow unchanged and remove reliance on the final sequential restoration statement.
269-286: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRequire the negative-path handlers to exist.
If
getHandlerreturnsundefined, optional calls skip both handlers. Thenot.toHaveBeenCalled()assertions then pass even whensetupDownloads()fails to register them. Assert that both handlers are functions before invoking them.🤖 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/downloads/main/integration.main.spec.ts` around lines 269 - 286, Update the test around getHandler in the non-existent-download case to assert that both showInFolderHandler and copyLinkHandler are functions before invoking them. Remove optional invocation so the test fails when setupDownloads does not register either IPC handler, while preserving the existing shell and clipboard non-call assertions.
🧹 Nitpick comments (3)
src/downloads/main/integration.main.spec.ts (2)
327-391: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winMake the simultaneous-download test deterministic.
The two calls run sequentially and use real
Date.now()values. They can receive the sameitemId, so one map entry can overwrite the other while the filename assertions still pass. Use deterministic timestamps, run both calls withPromise.all, and assert distinctitemIdvalues.🤖 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/downloads/main/integration.main.spec.ts` around lines 327 - 391, Update the “should handle multiple simultaneous downloads” test around handleWillDownloadEvent to mock deterministic Date.now() values, invoke both download handlers concurrently with Promise.all, and assert the dispatched DOWNLOAD_CREATED actions contain distinct itemId values in addition to their filenames.
394-429: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winVerify item-map cleanup directly.
The test checks only
DOWNLOAD_UPDATED, which occurs beforeitems.delete(itemId). It passes even if cleanup is removed. After completion, invoke a registered pause or cancel handler with the createditemIdand assert that the mock item is not called.🤖 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/downloads/main/integration.main.spec.ts` around lines 394 - 429, Update the “should properly clean up download items after completion” test to capture the created itemId and directly verify removal from tracking: after invoking doneListener, call a registered pause or cancel handler for that itemId, then assert the mock item was not called. Keep the existing completion dispatch assertion, but ensure the new check would fail if items.delete(itemId) were removed.src/app/main/buildAssets.main.spec.ts (1)
34-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert
buildAssetsbehavior instead of unrelated Node APIs.The test only requires the module and then tests
path.joinandfs.existsSync. It does not verify abuildAssetshelper or guarded entrypoint. Assert the module’s exported behavior, or rename the test and comment to state that it only checks module loading.🤖 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/app/main/buildAssets.main.spec.ts` around lines 34 - 42, Update the “buildAssets module load” test to assert behavior exported by the buildAssets module, including its helper or guarded entrypoint, instead of checking unrelated path.join and fs.existsSync APIs. If no buildAssets behavior is available to assert, rename the test and its comment to explicitly describe module-loading coverage.
🤖 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/documentViewer/main/ipc.spec.ts`:
- Line 5: Rename the main-process spec file from ipc.spec.ts to ipc.main.spec.ts
so it matches the required *.main.spec.ts discovery convention; leave its test
contents and the startDocumentViewerHandler import unchanged.
In `@src/servers/main/resolveServerUrl.spec.ts`:
- Around line 53-55: Rename src/servers/main/resolveServerUrl.spec.ts to
src/servers/main/resolveServerUrl.main.spec.ts and
src/servers/main/setupServers.spec.ts to
src/servers/main/setupServers.main.spec.ts so both main-process specs follow the
required Jest naming convention.
In `@src/ui/main/dock.main.spec.ts`:
- Around line 49-57: Strengthen the non-darwin test around dock.setUp() by
clearing watchCallbacks before setup and asserting its size remains zero
afterward. Keep the existing no-throw assertion, ensuring initialize does not
register watchers on Linux.
---
Outside diff comments:
In `@src/downloads/main/integration.main.spec.ts`:
- Around line 288-323: Update the test case around Date.now and the
pause/resume/cancel handler assertions so the original Date.now reference is
restored in a finally block, regardless of whether setup, handlers, or
assertions throw. Keep the existing test flow unchanged and remove reliance on
the final sequential restoration statement.
- Around line 269-286: Update the test around getHandler in the
non-existent-download case to assert that both showInFolderHandler and
copyLinkHandler are functions before invoking them. Remove optional invocation
so the test fails when setupDownloads does not register either IPC handler,
while preserving the existing shell and clipboard non-call assertions.
---
Nitpick comments:
In `@src/app/main/buildAssets.main.spec.ts`:
- Around line 34-42: Update the “buildAssets module load” test to assert
behavior exported by the buildAssets module, including its helper or guarded
entrypoint, instead of checking unrelated path.join and fs.existsSync APIs. If
no buildAssets behavior is available to assert, rename the test and its comment
to explicitly describe module-loading coverage.
In `@src/downloads/main/integration.main.spec.ts`:
- Around line 327-391: Update the “should handle multiple simultaneous
downloads” test around handleWillDownloadEvent to mock deterministic Date.now()
values, invoke both download handlers concurrently with Promise.all, and assert
the dispatched DOWNLOAD_CREATED actions contain distinct itemId values in
addition to their filenames.
- Around line 394-429: Update the “should properly clean up download items after
completion” test to capture the created itemId and directly verify removal from
tracking: after invoking doneListener, call a registered pause or cancel handler
for that itemId, then assert the mock item was not called. Keep the existing
completion dispatch assertion, but ensure the new check would fail if
items.delete(itemId) were removed.
🪄 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 Plus
Run ID: f36eef76-faeb-406f-9de4-5da45906f231
📒 Files selected for processing (23)
src/app/main/buildAssets.main.spec.tssrc/app/main/mainEntry.main.spec.tssrc/documentViewer/main/ipc.spec.tssrc/downloads/main/integration.main.spec.tssrc/i18n/__tests__/renderer.spec.tssrc/logViewerWindow/__tests__/logViewerWindow.spec.tsxsrc/notifications/__tests__/attentionDrawing.spec.tssrc/outlookCalendar/main/ipc.main.spec.tssrc/outlookCalendar/reducers/__tests__/outlookReducers.spec.tssrc/screenSharing/__tests__/screenSharePicker.spec.tsxsrc/servers/__tests__/fetchInfo.spec.tssrc/servers/main/preloadCoverage.spec.tssrc/servers/main/resolveServerUrl.spec.tssrc/servers/main/setupServers.spec.tssrc/ui/components/FailureImage.spec.tsxsrc/ui/components/ServersView/PdfContent.spec.tsxsrc/ui/components/ServersView/ServerPane.spec.tsxsrc/ui/components/SettingsView/features/moreSettings.spec.tsxsrc/ui/components/TopBar/index.spec.tsxsrc/ui/components/utils/TooltipProvider.spec.tsxsrc/ui/main/dock.main.spec.tssrc/ui/main/menuBar.main.spec.tssrc/videoCallWindow/main/ipc.main.spec.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- src/servers/tests/fetchInfo.spec.ts
- src/ui/components/utils/TooltipProvider.spec.tsx
- src/ui/components/ServersView/ServerPane.spec.tsx
- src/outlookCalendar/reducers/tests/outlookReducers.spec.ts
- src/ui/components/FailureImage.spec.tsx
- src/notifications/tests/attentionDrawing.spec.ts
- src/screenSharing/tests/screenSharePicker.spec.tsx
- src/ui/components/ServersView/PdfContent.spec.tsx
- src/servers/main/preloadCoverage.spec.ts
- src/i18n/tests/renderer.spec.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: check (macos-latest)
- GitHub Check: check (windows-latest)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript for new code unless explicitly told otherwise.
Use Fuselage components from@rocket.chat/fuselagefor UI work unless the design requires something Fuselage does not provide.
CheckTheme.d.tsfor valid color tokens before using Fuselage colors.
Verify library props, APIs, and tokens against official docs or local.d.tsfiles instead of assuming.
Use React functional components with hooks.
Redux actions follow FSA shape.
Use camelCase for file names and PascalCase for components.
Prefer clear names over unnecessary comments.
Prefer editing existing files over creating new abstractions unless the new abstraction removes real complexity or matches an existing pattern.
**/*.{ts,tsx}: Use TypeScript for all new code unless explicitly told otherwise.
Use Fuselage components for all UI work; create custom components only when Fuselage lacks the required functionality.
Import Fuselage components from@rocket.chat/fuselage.
Use only valid color tokens documented byTheme.d.ts.
Use optional chaining with fallbacks for platform-specific APIs, especially Linux-only process APIs such asprocess.getuid(),getgid(),geteuid(), andgetegid().
Use TypeScript strict mode.
Redux actions must follow the Flux Standard Action pattern.
Use camelCase for file names and PascalCase for component names.
Avoid unnecessary comments; prefer self-documenting code through clear naming.
Do not commit or push without explicit user permission.
Verify library APIs, props, tokens, and types against official documentation and.d.tsfiles instead of assuming they are valid.
Files:
src/app/main/mainEntry.main.spec.tssrc/app/main/buildAssets.main.spec.tssrc/ui/main/menuBar.main.spec.tssrc/ui/main/dock.main.spec.tssrc/servers/main/setupServers.spec.tssrc/documentViewer/main/ipc.spec.tssrc/ui/components/TopBar/index.spec.tsxsrc/downloads/main/integration.main.spec.tssrc/ui/components/SettingsView/features/moreSettings.spec.tsxsrc/servers/main/resolveServerUrl.spec.tssrc/outlookCalendar/main/ipc.main.spec.tssrc/logViewerWindow/__tests__/logViewerWindow.spec.tsxsrc/videoCallWindow/main/ipc.main.spec.ts
**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Renderer specs use
*.spec.ts/*.spec.tsx.
Files:
src/app/main/mainEntry.main.spec.tssrc/app/main/buildAssets.main.spec.tssrc/ui/main/menuBar.main.spec.tssrc/ui/main/dock.main.spec.tssrc/servers/main/setupServers.spec.tssrc/documentViewer/main/ipc.spec.tssrc/ui/components/TopBar/index.spec.tsxsrc/downloads/main/integration.main.spec.tssrc/ui/components/SettingsView/features/moreSettings.spec.tsxsrc/servers/main/resolveServerUrl.spec.tssrc/outlookCalendar/main/ipc.main.spec.tssrc/logViewerWindow/__tests__/logViewerWindow.spec.tsxsrc/videoCallWindow/main/ipc.main.spec.ts
**/*.main.spec.ts
📄 CodeRabbit inference engine (AGENTS.md)
Main-process specs use
*.main.spec.ts.Use
*.main.spec.tsfor main process tests.
Files:
src/app/main/mainEntry.main.spec.tssrc/app/main/buildAssets.main.spec.tssrc/ui/main/menuBar.main.spec.tssrc/ui/main/dock.main.spec.tssrc/downloads/main/integration.main.spec.tssrc/outlookCalendar/main/ipc.main.spec.tssrc/videoCallWindow/main/ipc.main.spec.ts
src/*/*/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Renderer specs must live in a Jest-matched nested path, such as
src/<module>/<subdir>/*.spec.ts(x); flatsrc/<module>/*.spec.tsfiles are not discovered by the currenttestMatch.
Files:
src/app/main/mainEntry.main.spec.tssrc/app/main/buildAssets.main.spec.tssrc/ui/main/menuBar.main.spec.tssrc/ui/main/dock.main.spec.tssrc/servers/main/setupServers.spec.tssrc/documentViewer/main/ipc.spec.tssrc/downloads/main/integration.main.spec.tssrc/servers/main/resolveServerUrl.spec.tssrc/outlookCalendar/main/ipc.main.spec.tssrc/logViewerWindow/__tests__/logViewerWindow.spec.tsxsrc/videoCallWindow/main/ipc.main.spec.ts
**/*.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
*.spec.tsfor renderer process tests.
Files:
src/app/main/mainEntry.main.spec.tssrc/app/main/buildAssets.main.spec.tssrc/ui/main/menuBar.main.spec.tssrc/ui/main/dock.main.spec.tssrc/servers/main/setupServers.spec.tssrc/documentViewer/main/ipc.spec.tssrc/downloads/main/integration.main.spec.tssrc/servers/main/resolveServerUrl.spec.tssrc/outlookCalendar/main/ipc.main.spec.tssrc/videoCallWindow/main/ipc.main.spec.ts
src/**/*.{spec.ts,spec.tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Renderer test files should be placed in nested module paths such as
src/<module>/<subdir>/*.spec.ts(x)so Jest discovers them.
Files:
src/app/main/mainEntry.main.spec.tssrc/app/main/buildAssets.main.spec.tssrc/ui/main/menuBar.main.spec.tssrc/ui/main/dock.main.spec.tssrc/servers/main/setupServers.spec.tssrc/documentViewer/main/ipc.spec.tssrc/ui/components/TopBar/index.spec.tsxsrc/downloads/main/integration.main.spec.tssrc/ui/components/SettingsView/features/moreSettings.spec.tsxsrc/servers/main/resolveServerUrl.spec.tssrc/outlookCalendar/main/ipc.main.spec.tssrc/logViewerWindow/__tests__/logViewerWindow.spec.tsxsrc/videoCallWindow/main/ipc.main.spec.ts
**/*.{tsx,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use React functional components with hooks.
Files:
src/ui/components/TopBar/index.spec.tsxsrc/ui/components/SettingsView/features/moreSettings.spec.tsxsrc/logViewerWindow/__tests__/logViewerWindow.spec.tsx
src/outlookCalendar/**/*.{ts,tsx}
📄 CodeRabbit inference engine (src/outlookCalendar/AGENTS.md)
src/outlookCalendar/**/*.{ts,tsx}: UsecreateClassifiedError()fromerrorClassification.tsfor user-facing errors to provide error categorization, user-friendly messages, and structured error context
Always use outlookError() for errors as it logs regardless of verbose mode settings, ensuring errors are always visible to users
Files:
src/outlookCalendar/main/ipc.main.spec.ts
🧠 Learnings (2)
📚 Learning: 2026-06-26T18:14:11.817Z
Learnt from: jeanfbrito
Repo: RocketChat/Rocket.Chat.Electron PR: 3358
File: src/ui/components/SettingsView/features/E2ePdfPreviewSizeLimit.tsx:47-55
Timestamp: 2026-06-26T18:14:11.817Z
Learning: In the Rocket.Chat Electron App SettingsView features under `src/ui/components/SettingsView/features/`, treat full-width selects/inputs (including full-width numeric inputs) as intentional for the stacked label/description layout. Per the UXDQA Figma spec (and macOS 1:1 verification), reviews should not flag these as layout regressions as long as they match the expected form-column stretching behavior.
Applied to files:
src/ui/components/SettingsView/features/moreSettings.spec.tsx
📚 Learning: 2026-06-26T18:14:13.838Z
Learnt from: jeanfbrito
Repo: RocketChat/Rocket.Chat.Electron PR: 3358
File: src/ui/components/SettingsView/features/ToggleField.tsx:1-8
Timestamp: 2026-06-26T18:14:13.838Z
Learning: In Rocket.Chat Electron App settings field UIs that use the Fuselage three-tier pattern, keep the `FieldLabel` / `FieldDescription` / `FieldHint` structure separate. Use `FieldDescription` for the regular secondary body text, and reserve `FieldHint` for the smaller, dimmer subline content (e.g., restart caveats). Do not collapse `FieldDescription` and `FieldHint` into a single hint tier, as this violates the intended UXDQA spec.
Applied to files:
src/ui/components/SettingsView/features/moreSettings.spec.tsx
🔇 Additional comments (13)
src/documentViewer/main/ipc.spec.ts (1)
56-58: LGTM!Also applies to: 156-178
src/outlookCalendar/main/ipc.main.spec.ts (1)
5-7: LGTM!Also applies to: 114-116, 204-205, 243-243, 328-372, 437-451
src/ui/components/SettingsView/features/moreSettings.spec.tsx (1)
3-58: LGTM!Also applies to: 188-229
src/ui/components/TopBar/index.spec.tsx (1)
34-57: LGTM!src/ui/main/menuBar.main.spec.ts (1)
374-374: 🎯 Functional CorrectnessNo duplicate
errorsdeclarations are present.The two
const errorsdeclarations are in separate handler blocks, so there is no compile-time redeclaration issue.> Likely an incorrect or invalid review comment.src/app/main/buildAssets.main.spec.ts (1)
26-32: LGTM!src/app/main/mainEntry.main.spec.ts (1)
1-34: LGTM!src/downloads/main/integration.main.spec.ts (1)
1-16: LGTM!Also applies to: 36-41, 51-52
src/logViewerWindow/__tests__/logViewerWindow.spec.tsx (1)
4-5: LGTM!Also applies to: 27-27, 42-42, 60-78, 107-109, 161-171, 173-187
src/videoCallWindow/main/ipc.main.spec.ts (4)
2-21: LGTM!Also applies to: 37-43
71-78: LGTM!Also applies to: 174-175, 184-187, 214-217, 279-290, 300-300
331-334: LGTM!Also applies to: 350-357
379-487: LGTM!Also applies to: 907-913, 961-984
|
|
||
| import { SERVER_DOCUMENT_VIEWER_OPEN_URL } from '../../servers/actions'; | ||
| import { WEBVIEW_PDF_VIEWER_ATTACHED } from '../../ui/actions'; | ||
| import { startDocumentViewerHandler } from '../ipc'; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Rename this main-process spec.
src/documentViewer/main/ipc.spec.ts does not match the required *.main.spec.ts name. The main-process Jest harness may not discover or route this test correctly. Rename it to src/documentViewer/main/ipc.main.spec.ts.
As per coding guidelines, “Main-process specs use *.main.spec.ts.”
🤖 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/documentViewer/main/ipc.spec.ts` at line 5, Rename the main-process spec
file from ipc.spec.ts to ipc.main.spec.ts so it matches the required
*.main.spec.ts discovery convention; leave its test contents and the
startDocumentViewerHandler import unchanged.
Source: Coding guidelines
| const [, status, error] = await resolveServerUrl( | ||
| 'https://open.rocket.chat' | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Rename these main-process specs.
These files test main-process modules, but their .spec.ts suffix classifies them as renderer specs. Rename them to the required .main.spec.ts form so Jest uses the main-process test routing.
src/servers/main/resolveServerUrl.spec.ts#L53-L55: rename tosrc/servers/main/resolveServerUrl.main.spec.ts.src/servers/main/setupServers.spec.ts#L1-L8: rename tosrc/servers/main/setupServers.main.spec.ts.
As per coding guidelines, “Main-process specs use *.main.spec.ts.”
📍 Affects 2 files
src/servers/main/resolveServerUrl.spec.ts#L53-L55(this comment)src/servers/main/setupServers.spec.ts#L1-L8
🤖 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/servers/main/resolveServerUrl.spec.ts` around lines 53 - 55, Rename
src/servers/main/resolveServerUrl.spec.ts to
src/servers/main/resolveServerUrl.main.spec.ts and
src/servers/main/setupServers.spec.ts to
src/servers/main/setupServers.main.spec.ts so both main-process specs follow the
required Jest naming convention.
Source: Coding guidelines
| it('no-ops initialize on non-darwin platforms', () => { | ||
| Object.defineProperty(process, 'platform', { | ||
| value: 'linux', | ||
| configurable: true, | ||
| }); | ||
| // eslint-disable-next-line @typescript-eslint/no-var-requires | ||
| const dock = require('./dock').default; | ||
| expect(() => dock.setUp()).not.toThrow(); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Verify the non-macOS no-op behavior.
This test only verifies that dock.setUp() does not throw. It passes if initialize() registers watchers on Linux. Clear watchCallbacks before setup and assert that its size remains zero.
Proposed test update
it('no-ops initialize on non-darwin platforms', () => {
Object.defineProperty(process, 'platform', {
value: 'linux',
configurable: true,
});
+ watchCallbacks.clear();
// eslint-disable-next-line `@typescript-eslint/no-var-requires`
const dock = require('./dock').default;
- expect(() => dock.setUp()).not.toThrow();
+ dock.setUp();
+
+ expect(watchCallbacks.size).toBe(0);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('no-ops initialize on non-darwin platforms', () => { | |
| Object.defineProperty(process, 'platform', { | |
| value: 'linux', | |
| configurable: true, | |
| }); | |
| // eslint-disable-next-line @typescript-eslint/no-var-requires | |
| const dock = require('./dock').default; | |
| expect(() => dock.setUp()).not.toThrow(); | |
| }); | |
| it('no-ops initialize on non-darwin platforms', () => { | |
| Object.defineProperty(process, 'platform', { | |
| value: 'linux', | |
| configurable: true, | |
| }); | |
| watchCallbacks.clear(); | |
| // eslint-disable-next-line `@typescript-eslint/no-var-requires` | |
| const dock = require('./dock').default; | |
| dock.setUp(); | |
| expect(watchCallbacks.size).toBe(0); | |
| }); |
🤖 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/main/dock.main.spec.ts` around lines 49 - 57, Strengthen the
non-darwin test around dock.setUp() by clearing watchCallbacks before setup and
asserting its size remains zero afterward. Keep the existing no-throw assertion,
ensuring initialize does not register watchers on Linux.
CI's Lint step (yarn lint = eslint + tsc --noEmit) was failing on all
three platforms. Two root causes:
- 33 auto-fixable prettier/import-order violations across specs
(fixed via `eslint --fix`), plus one naming-convention error in
settingsToggles.spec.tsx (a `Component` prop key/destructure
conflicted with camelCase parameter rules — renamed the field to
`component` and kept a capitalized local alias for JSX usage).
- 5 spec files with no top-level import/export statement, causing
TypeScript to treat them as global scripts rather than modules.
Their same-named top-level consts (`handlers`, `dispatch`, `select`,
`watchCallbacks`, `getRootWindow`) collided across files under
whole-project `tsc --noEmit`, even though each runs fine in
isolation under Jest. Added `export {}` to force module scope.
Also fixed a real regression introduced by the eslint --fix pass:
it hoisted the `setupUpdates` import in setupUpdates.spec.ts above
the `autoUpdater` const its `electron-updater` mock factory closes
over, causing "Cannot access 'autoUpdater' before initialization" at
runtime. Restored the import to its required position below the
const and mocks, with a comment and scoped eslint-disable explaining
why the ordering can't be "corrected" by tooling.
Verified: `yarn lint` exits 0, full `yarn test` is green (217 suites,
1972 tests, 0 failures).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/updates/main/setupUpdates.spec.ts`:
- Around line 76-79: Rename the test file from setupUpdates.spec.ts to
setupUpdates.main.spec.ts so Jest classifies it as a main-process spec; preserve
its contents and import ordering.
🪄 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 Plus
Run ID: 222ce113-7a94-4ecf-9354-603363f38685
📒 Files selected for processing (21)
src/logViewerWindow/logViewerWindow.tsxsrc/logViewerWindow/main/ipc.main.spec.tssrc/notifications/main/setup.main.spec.tssrc/outlookCalendar/main/ipc.main.spec.tssrc/servers/main.spec.tssrc/ui/components/App.spec.tsxsrc/ui/components/CertificatesManager/CertificatesManager.spec.tsxsrc/ui/components/ServerInfoModal/index.spec.tsxsrc/ui/components/ServersView/DocumentViewer.spec.tsxsrc/ui/components/ServersView/ErrorView.spec.tsxsrc/ui/components/ServersView/MarkdownContent.spec.tsxsrc/ui/components/SettingsView/features/settingsToggles.spec.tsxsrc/ui/components/SettingsView/tabs.spec.tsxsrc/ui/components/utils/ErrorCatcher.spec.tsxsrc/ui/components/utils/getServerInitials.spec.tssrc/ui/main/__tests__/rootWindowGeometry.main.spec.tssrc/ui/main/dock.main.spec.tssrc/ui/main/touchBar.main.spec.tssrc/ui/main/trayIcon.main.spec.tssrc/updates/main/setupUpdates.spec.tssrc/userPresence/main/setup.main.spec.ts
💤 Files with no reviewable changes (1)
- src/ui/main/tests/rootWindowGeometry.main.spec.ts
🚧 Files skipped from review as they are similar to previous changes (17)
- src/ui/components/ServerInfoModal/index.spec.tsx
- src/ui/components/SettingsView/features/settingsToggles.spec.tsx
- src/ui/components/utils/getServerInitials.spec.ts
- src/ui/components/utils/ErrorCatcher.spec.tsx
- src/userPresence/main/setup.main.spec.ts
- src/ui/components/ServersView/ErrorView.spec.tsx
- src/logViewerWindow/logViewerWindow.tsx
- src/ui/components/CertificatesManager/CertificatesManager.spec.tsx
- src/ui/components/ServersView/MarkdownContent.spec.tsx
- src/ui/components/App.spec.tsx
- src/servers/main.spec.ts
- src/ui/components/ServersView/DocumentViewer.spec.tsx
- src/logViewerWindow/main/ipc.main.spec.ts
- src/notifications/main/setup.main.spec.ts
- src/ui/components/SettingsView/tabs.spec.tsx
- src/outlookCalendar/main/ipc.main.spec.ts
- src/ui/main/touchBar.main.spec.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: check (macos-latest)
- GitHub Check: check (ubuntu-latest)
- GitHub Check: check (windows-latest)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript for new code unless explicitly told otherwise.
Use Fuselage components from@rocket.chat/fuselagefor UI work unless the design requires something Fuselage does not provide.
CheckTheme.d.tsfor valid color tokens before using Fuselage colors.
Verify library props, APIs, and tokens against official docs or local.d.tsfiles instead of assuming.
Use React functional components with hooks.
Redux actions follow FSA shape.
Use camelCase for file names and PascalCase for components.
Prefer clear names over unnecessary comments.
Prefer editing existing files over creating new abstractions unless the new abstraction removes real complexity or matches an existing pattern.
**/*.{ts,tsx}: Use TypeScript for all new code unless explicitly told otherwise.
Use Fuselage components for all UI work; create custom components only when Fuselage lacks the required functionality.
Import Fuselage components from@rocket.chat/fuselage.
Use only valid color tokens documented byTheme.d.ts.
Use optional chaining with fallbacks for platform-specific APIs, especially Linux-only process APIs such asprocess.getuid(),getgid(),geteuid(), andgetegid().
Use TypeScript strict mode.
Redux actions must follow the Flux Standard Action pattern.
Use camelCase for file names and PascalCase for component names.
Avoid unnecessary comments; prefer self-documenting code through clear naming.
Do not commit or push without explicit user permission.
Verify library APIs, props, tokens, and types against official documentation and.d.tsfiles instead of assuming they are valid.
Files:
src/updates/main/setupUpdates.spec.tssrc/ui/main/dock.main.spec.tssrc/ui/main/trayIcon.main.spec.ts
**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Renderer specs use
*.spec.ts/*.spec.tsx.
Files:
src/updates/main/setupUpdates.spec.tssrc/ui/main/dock.main.spec.tssrc/ui/main/trayIcon.main.spec.ts
src/*/*/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Renderer specs must live in a Jest-matched nested path, such as
src/<module>/<subdir>/*.spec.ts(x); flatsrc/<module>/*.spec.tsfiles are not discovered by the currenttestMatch.
Files:
src/updates/main/setupUpdates.spec.tssrc/ui/main/dock.main.spec.tssrc/ui/main/trayIcon.main.spec.ts
**/*.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
*.spec.tsfor renderer process tests.
Files:
src/updates/main/setupUpdates.spec.tssrc/ui/main/dock.main.spec.tssrc/ui/main/trayIcon.main.spec.ts
src/**/*.{spec.ts,spec.tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Renderer test files should be placed in nested module paths such as
src/<module>/<subdir>/*.spec.ts(x)so Jest discovers them.
Files:
src/updates/main/setupUpdates.spec.tssrc/ui/main/dock.main.spec.tssrc/ui/main/trayIcon.main.spec.ts
**/*.main.spec.ts
📄 CodeRabbit inference engine (AGENTS.md)
Main-process specs use
*.main.spec.ts.Use
*.main.spec.tsfor main process tests.
Files:
src/ui/main/dock.main.spec.tssrc/ui/main/trayIcon.main.spec.ts
🔇 Additional comments (3)
src/ui/main/dock.main.spec.ts (1)
1-2: LGTM!src/ui/main/trayIcon.main.spec.ts (1)
1-2: LGTM!src/updates/main/setupUpdates.spec.ts (1)
3-12: LGTM!
| // Must stay below the `autoUpdater` const and jest.mock('electron-updater', ...) | ||
| // above: importing '../main' pulls in electron-updater, whose mock factory | ||
| // closes over `autoUpdater` — hoisting this import breaks that initialization order. | ||
| // eslint-disable-next-line import/first |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the main-process spec suffix.
Rename src/updates/main/setupUpdates.spec.ts to src/updates/main/setupUpdates.main.spec.ts so Jest can classify it consistently with other main-process tests.
As per coding guidelines, main-process specs use *.main.spec.ts.
🤖 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/updates/main/setupUpdates.spec.ts` around lines 76 - 79, Rename the test
file from setupUpdates.spec.ts to setupUpdates.main.spec.ts so Jest classifies
it as a main-process spec; preserve its contents and import ordering.
Source: Coding guidelines
CI's ubuntu-latest and windows-latest check jobs failed while macos-latest passed, revealing three real platform-dependent test bugs: - menuBar.main.spec.ts: a new test asserted on the darwin-only 'about' appMenu item without mocking process.platform, so it failed on non-darwin runners. Switched to windowMenu's 'settings' item, which exercises the identical show-if-hidden-then-focus behavior but is registered on every platform. (selectMenuBarTemplate is memoized via createSelector, so a same-test process.platform override can't force recomputation of an already-cached platform-gated item anyway — confirmed by reproducing the failure with a forced-linux variant.) - setupUpdates.spec.ts: isUpdatingAllowed is computed by the real loadConfiguration() selector directly from process.platform/ process.mas/process.windowsStore, never from the mocked store state. On Linux CI (no APPIMAGE env) this is always false, short-circuiting setupUpdates() before it wires up autoUpdater or any listeners. Pinned process.platform to 'win32' (windowsStore: false) in beforeEach/afterEach so the allowed branch runs deterministically on every CI runner. Verified by reproducing the exact 3 CI failures locally with a forced-linux variant, then confirming the fix passes. - logging/__tests__/cleanup.spec.ts (pre-existing, not touched by earlier commits in this branch): asserted unlinkSync was called with a hardcoded forward-slash path, but the real implementation builds the path with path.join(), which uses backslashes on Windows. Switched the assertion to path.join() as well. Verified: yarn lint exits 0, full yarn test is green (217 suites, 1972 tests, 0 failures) locally.
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)
src/logging/__tests__/cleanup.spec.ts (1)
2-6: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRename this file as a main-process spec.
src/logging/cleanup.tscalls Electronapp.getPath('logs'), so this is a main-process test. Renamesrc/logging/__tests__/cleanup.spec.tstosrc/logging/__tests__/cleanup.main.spec.tsso Jest applies the correct process-specific test pattern.As per coding guidelines, main-process specs use
*.main.spec.ts.🤖 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/logging/__tests__/cleanup.spec.ts` around lines 2 - 6, Rename the cleanup test file containing cleanupOldLogs to cleanup.main.spec.ts so Jest recognizes it as a main-process spec, without changing its test contents.Source: Coding guidelines
🤖 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 `@src/logging/__tests__/cleanup.spec.ts`:
- Around line 2-6: Rename the cleanup test file containing cleanupOldLogs to
cleanup.main.spec.ts so Jest recognizes it as a main-process spec, without
changing its test contents.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c69d479f-8adf-43fc-95b6-e4d08bdf24ee
📒 Files selected for processing (3)
src/logging/__tests__/cleanup.spec.tssrc/ui/main/menuBar.main.spec.tssrc/updates/main/setupUpdates.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/updates/main/setupUpdates.spec.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: check (macos-latest)
- GitHub Check: check (ubuntu-latest)
- GitHub Check: check (windows-latest)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript for new code unless explicitly told otherwise.
Use Fuselage components from@rocket.chat/fuselagefor UI work unless the design requires something Fuselage does not provide.
CheckTheme.d.tsfor valid color tokens before using Fuselage colors.
Verify library props, APIs, and tokens against official docs or local.d.tsfiles instead of assuming.
Use React functional components with hooks.
Redux actions follow FSA shape.
Use camelCase for file names and PascalCase for components.
Prefer clear names over unnecessary comments.
Prefer editing existing files over creating new abstractions unless the new abstraction removes real complexity or matches an existing pattern.
**/*.{ts,tsx}: Use TypeScript for all new code unless explicitly told otherwise.
Use Fuselage components for all UI work; create custom components only when Fuselage lacks the required functionality.
Import Fuselage components from@rocket.chat/fuselage.
Use only valid color tokens documented byTheme.d.ts.
Use optional chaining with fallbacks for platform-specific APIs, especially Linux-only process APIs such asprocess.getuid(),getgid(),geteuid(), andgetegid().
Use TypeScript strict mode.
Redux actions must follow the Flux Standard Action pattern.
Use camelCase for file names and PascalCase for component names.
Avoid unnecessary comments; prefer self-documenting code through clear naming.
Do not commit or push without explicit user permission.
Verify library APIs, props, tokens, and types against official documentation and.d.tsfiles instead of assuming they are valid.
Files:
src/logging/__tests__/cleanup.spec.tssrc/ui/main/menuBar.main.spec.ts
**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Renderer specs use
*.spec.ts/*.spec.tsx.
Files:
src/logging/__tests__/cleanup.spec.tssrc/ui/main/menuBar.main.spec.ts
src/*/*/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Renderer specs must live in a Jest-matched nested path, such as
src/<module>/<subdir>/*.spec.ts(x); flatsrc/<module>/*.spec.tsfiles are not discovered by the currenttestMatch.
Files:
src/logging/__tests__/cleanup.spec.tssrc/ui/main/menuBar.main.spec.ts
**/*.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
*.spec.tsfor renderer process tests.
Files:
src/logging/__tests__/cleanup.spec.tssrc/ui/main/menuBar.main.spec.ts
src/**/*.{spec.ts,spec.tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Renderer test files should be placed in nested module paths such as
src/<module>/<subdir>/*.spec.ts(x)so Jest discovers them.
Files:
src/logging/__tests__/cleanup.spec.tssrc/ui/main/menuBar.main.spec.ts
**/*.main.spec.ts
📄 CodeRabbit inference engine (AGENTS.md)
Main-process specs use
*.main.spec.ts.Use
*.main.spec.tsfor main process tests.
Files:
src/ui/main/menuBar.main.spec.ts
🔇 Additional comments (2)
src/logging/__tests__/cleanup.spec.ts (1)
85-85: LGTM!Also applies to: 129-129
src/ui/main/menuBar.main.spec.ts (1)
1-194: LGTM!Also applies to: 196-251, 253-317, 319-395, 397-428, 430-452
1 participant
Summary
collectCoverageFrom: src/**/*.{ts,tsx}denominator — no narrowing of the coverage surface.testMatch(nested__tests__/main//*.main.specpaths). Flat orphan specs were silently never running.preloadCoverage.spec.ts), since renderer preloads trip IstanbulEvalErrorunder the electron runner.validateVideoCallUrl,logFormatters) and fixes ErrorView so(isFailed || isReloading)correctly renders the failure UI (operator-precedence bug).docs/COVERAGE.md.Measured results (local
yarn test:coverage)Production changes (small)
ErrorView.tsx: fix short-circuit so failed/reloading states actually show the error chrome.validateVideoCallUrl.ts/logFormatters.ts: extract pure logic already used by the product paths (behavior unchanged).Test plan
yarn test:coveragegreen locally;coverage/coverage-summary.json→total.lines.pct >= 70with fullsrc/**collectvalidate-pr/ Codecov informational report on this PRSummary by CodeRabbit
Bug Fixes
Documentation
Tests