feat: add Jest testing setup with sign-in view tests and CI workflow#4
feat: add Jest testing setup with sign-in view tests and CI workflow#4
Conversation
WalkthroughIntroduces a CI workflow for Node-based lint/build on main and PRs, adds Jest testing infrastructure (config, setup, scripts, dev dependencies), adds a SignInView unit test, and inserts explanatory comments in the SignInView component. No runtime logic or public API changes; no artifacts are produced by CI. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Tester as Jest Test
participant RTV as SignInView (RTL)
participant Auth as AuthClient (mock)
participant Router as next/navigation (mock)
Tester->>RTV: render()
Tester->>RTV: type email/password
Tester->>RTV: click "Sign In"
RTV->>Auth: signIn.email({ email, password, onSuccess })
note over Auth: Mock immediately calls onSuccess
Auth-->>RTV: onSuccess()
RTV->>Router: push("/")
Tester->>Tester: expect(Router.push).toHaveBeenCalledWith("/")
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
package.json (1)
1-86: Require Babel transform dependencies and a CI test scriptThe verification shows that neither
babel-jestnor@babel/coreare present indevDependencies, and there is notest:ciscript. Without these, Jest won’t handle Next.js/Babel transforms correctly, and CI cannot run tests in CI mode.Please update package.json accordingly:
• Add Babel transform dependencies to
devDependencies
• Define atest:ciscript that runs Jest in CI mode
• Re-run tests locally under Node 22 to confirm everything passesSuggested diff:
devDependencies: { + "babel-jest": "^29.7.0", + "@babel/core": "^7.23.0", "drizzle-kit": "^0.31.1", "eslint": "^9", @@ scripts: { "dev": "next dev", "build": "next build", "start": "next start", "lint": "next lint", "test": "jest", + "test:ci": "jest --ci", "test:watch": "jest --watch", "db:push": "drizzle-kit push", "db:studio": "drizzle-kit studio" }Then install the new deps and confirm:
npm install npm run test:ciEnsure all tests pass on Node 22 before merging.
src/modules/auth/ui/views/sign-in-view.tsx (1)
50-70: Pending state never set to true; also not reset on errorCurrently you only call setPending(false) on success. The button is disabled based on pending, so the UI never reflects “in flight”, and pending is not reset on error.
const onSubmit = async (data: z.infer<typeof formSchema>) => { - //removes old error messages + // removes old error messages setError(null); + setPending(true); authClient.signIn.email( { email: data.email, password: data.password, }, { onSuccess: () => { setPending(false); router.push("/"); // App Router's push method is compatible with this usage }, onError: ({error}) => { setError(error.message); + setPending(false); } } ); };
🧹 Nitpick comments (12)
.github/workflows/ci.yml (2)
9-19: Optional: add a Node matrix to de-risk environment driftGiven Next 15 and React 19, verify on Node 20 and 22. This reduces “works locally, fails in prod” risk.
Proposed snippet (if you want to adopt a matrix):
jobs: - build: + build: runs-on: ubuntu-latest + strategy: + matrix: + node-version: [20.x, 22.x] steps: - uses: actions/checkout@v4 - uses: actions/setup-node@v4 with: - node-version: 22 + node-version: ${{ matrix.node-version }} cache: 'npm' - run: npm ci - run: npm run lint - run: npm test -- --ci - run: npm run build
20-20: Trailing space and missing newline at EOFYAMLlint flagged trailing spaces and lack of terminal newline on Line 20.
- +package.json (1)
10-11: Add a CI-friendly test scriptProvide a dedicated script for CI to enable coverage and non-interactive mode. This pairs with the CI step you’ll add.
"lint": "next lint", - "test": "jest", - "test:watch": "jest --watch", + "test": "jest", + "test:ci": "jest --ci --coverage", + "test:watch": "jest --watch",src/modules/auth/ui/views/sign-in-view.tsx (3)
25-29: Fix comment typo and clarity“heave @ domain” → “have @Domain”. Minor, but it’s in a prominent spot.
- //zod checks that email must be a string and look like a real email (heave @ domain) + // zod checks that email must be a string and look like a real email (must have @domain) - //password must be a string and can't be empty + // password must be a string and can't be empty
127-131: Tailwind class casing: border-noneTailwind utilities are lowercase. “border-None” won’t apply. Use border-none.
- <Alert className="bg-destructive/10 border-None"> + <Alert className="bg-destructive/10 border-none">
134-136: Tailwind utility typo: after:inset-0“insert-0” → “inset-0” to position the pseudo-element.
- <div className="after:border-border relative text-center text-sm after:absolute - after:insert-0 after:top-1/2 after:z-0 after:flex after:items-center after:border-t"> + <div className="after:border-border relative text-center text-sm after:absolute + after:inset-0 after:top-1/2 after:z-0 after:flex after:items-center after:border-t">jest.setup.js (1)
5-13: Mocking next/router is likely unused; keep only if you have Pages Router codeThe app uses App Router imports (next/navigation). Keeping this mock is harmless but adds noise. Consider removing unless you have legacy Pages components under test.
jest.config.js (1)
6-16: Babel transform is fine, but ensure deps exist; or switch to next/jest (recommended)Current config requires babel-jest and @babel/core (missing in package.json—see my other comment). Alternatively, use Next’s official next/jest to generate a config that tracks Next upgrades.
Option A (keep Babel): just add the missing devDependencies (see package.json comment).
Option B (recommended): switch to next/jest:
-//uses jsdom to simulate a browser environment -//points to a setup file -//ignores node_modules and .next directories - -module.exports = { - testEnvironment: 'jsdom', - setupFilesAfterEnv: ['<rootDir>/jest.setup.js'], - testPathIgnorePatterns: ['<rootDir>/node_modules/', '<rootDir>/.next/'], - moduleNameMapper: { - '^@/(.*)$': '<rootDir>/src/$1', - }, - transform: { - '^.+\\.(js|jsx|ts|tsx)$': ['babel-jest', { presets: ['next/babel'] }], - }, - }; +const nextJest = require('next/jest'); + +const createJestConfig = nextJest({ dir: './' }); + +const customJestConfig = { + testEnvironment: 'jsdom', + setupFilesAfterEnv: ['<rootDir>/jest.setup.js'], + testPathIgnorePatterns: ['<rootDir>/node_modules/', '<rootDir>/.next/'], + moduleNameMapper: { '^@/(.*)$': '<rootDir>/src/$1' }, +}; + +module.exports = createJestConfig(customJestConfig);src/__tests__/sign-in-view.test.tsx (4)
42-49: Use userEvent.setup() for realistic interactions and better async handlinguserEvent.setup() is the recommended pattern; it also gives you tighter control over async flushing and timers.
- //fill in the form - await userEvent.type(screen.getByLabelText(/email/i), 'test@example.com'); - await userEvent.type(screen.getByLabelText(/password/i), 'password'); - await userEvent.click(screen.getByRole('button', { name: /sign in/i })); + //fill in the form + const user = userEvent.setup(); + await user.type(screen.getByLabelText(/email/i), 'test@example.com'); + await user.type(screen.getByLabelText(/password/i), 'password'); + await user.click(screen.getByRole('button', { name: /sign in/i }));
29-29: Reset mocks between tests to prevent cross-test leakageClear calls/state between tests, especially once router mocks are shared.
describe('SignInView', () => { + afterEach(() => { + jest.clearAllMocks(); + });
18-27: Add error-path coverage by overriding the mocked auth call per testYour happy-path mock is fine. Add a test that triggers onError to assert error UI is rendered and navigation doesn’t occur.
Append this test near the end of the suite:
it('redirects to home on successful sign in', async()=> { render(<SignInView />); @@ - }); + }); + + it('shows an error and does not navigate on failed sign in', async () => { + const { authClient } = require('@/lib/auth-client'); + (authClient.signIn.email as jest.Mock).mockImplementationOnce( + (_data: any, { onError }: { onError: ({ error }: { error: Error }) => void }) => { + onError({ error: new Error('Invalid credentials') }); + } + ); + + render(<SignInView />); + const user = userEvent.setup(); + await user.type(screen.getByLabelText(/email/i), 'test@example.com'); + await user.type(screen.getByLabelText(/password/i), 'Password123!'); + await user.click(screen.getByRole('button', { name: /sign in/i })); + + expect(await screen.findByText('Invalid credentials')).toBeInTheDocument(); + expect(pushMock).not.toHaveBeenCalled(); + });
1-4: Optional cleanup: remove unused React import and jest-dom import in testsThe project’s Jest configuration (
jest.config.js) confirms thatjest.setup.jsis loaded viasetupFilesAfterEnv, and that file already imports@testing-library/jest-dom. Additionally, there’s no JSX in this test file, so theReactimport is unnecessary.File:
src/__tests__/sign-in-view.test.tsx
Lines: 1-4Suggested diff:
-import React from 'react'; -import '@testing-library/jest-dom'; +// jest-dom matchers are loaded via jest.setup.js
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
.github/workflows/ci.yml(1 hunks)jest.config.js(1 hunks)jest.setup.js(1 hunks)package.json(2 hunks)src/__tests__/sign-in-view.test.tsx(1 hunks)src/modules/auth/ui/views/sign-in-view.tsx(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/__tests__/sign-in-view.test.tsx (1)
src/modules/auth/ui/views/sign-in-view.tsx (1)
SignInView(32-175)
src/modules/auth/ui/views/sign-in-view.tsx (2)
src/modules/auth/ui/views/sign-up-view.tsx (3)
router(36-210)data(53-76)FormItem(116-124)src/app/(auth)/sign-in/page.tsx (1)
console(4-7)
🪛 actionlint (1.7.7)
.github/workflows/ci.yml
13-13: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
14-14: the runner of "actions/setup-node@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 YAMLlint (1.37.1)
.github/workflows/ci.yml
[warning] 16-16: wrong indentation: expected 18 but found 16
(indentation)
[error] 20-20: no new line character at the end of file
(new-line-at-end-of-file)
[error] 20-20: trailing spaces
(trailing-spaces)
🔇 Additional comments (1)
src/__tests__/sign-in-view.test.tsx (1)
31-39: LGTM: baseline render test covers key affordancesGood coverage for heading, labeled inputs, and primary action. This guards accidental regressions in the form scaffold.
| - uses: actions/checkout@v3 | ||
| - uses: actions/setup-node@v3 | ||
| with: | ||
| node-version: 22 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update actions to v4 and fix indentation; add caching
actionlint flags v3 runners as too old. Also YAMLlint reports wrong indentation at Line 16. Move to v4 and add npm cache for speed.
- - uses: actions/checkout@v3
- - uses: actions/setup-node@v3
+ - uses: actions/checkout@v4
+ - uses: actions/setup-node@v4
with:
- node-version: 22
+ node-version: 22
+ cache: 'npm'📝 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.
| - uses: actions/checkout@v3 | |
| - uses: actions/setup-node@v3 | |
| with: | |
| node-version: 22 | |
| - uses: actions/checkout@v4 | |
| - uses: actions/setup-node@v4 | |
| with: | |
| node-version: 22 | |
| cache: 'npm' |
🧰 Tools
🪛 actionlint (1.7.7)
13-13: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
14-14: the runner of "actions/setup-node@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 YAMLlint (1.37.1)
[warning] 16-16: wrong indentation: expected 18 but found 16
(indentation)
🤖 Prompt for AI Agents
.github/workflows/ci.yml around lines 13 to 16: the workflow uses
actions/checkout@v3 and actions/setup-node@v3 with an incorrectly indented
node-version line; update both actions to @v4, correct the indentation so the
node-version is nested under the setup-node step's with: block, and add an npm
cache step (or use actions/cache@v4 to cache ~/.npm or node_modules) to speed
CI. Ensure the YAML keys align properly (uses, with, node-version) and that
cache is placed before install to restore and after install to save.
| - run: npm ci | ||
| - run: npm run lint | ||
| - run: npm run build |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Run tests in CI (not just lint/build)
The PR introduces Jest, but CI doesn’t execute tests. Add a test step (CI mode) so regressions are caught before merge.
- run: npm ci
- run: npm run lint
+ - run: npm test -- --ci
- run: npm run build📝 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.
| - run: npm ci | |
| - run: npm run lint | |
| - run: npm run build | |
| - run: npm ci | |
| - run: npm run lint | |
| - run: npm test -- --ci | |
| - run: npm run build |
🤖 Prompt for AI Agents
In .github/workflows/ci.yml around lines 17 to 19, the workflow only runs npm
ci, lint and build but doesn’t run Jest tests; add a CI test step (e.g. run the
repo’s test script with CI mode) between install and build so tests execute in
GitHub Actions (for example run: npm test -- --ci or npm run test -- --ci),
ensure the step fails the job on test failures and is ordered before the build
step.
| // Mock next/navigation | ||
| jest.mock('next/navigation', () => ({ | ||
| useRouter: () => ({ | ||
| push: jest.fn(), | ||
| replace: jest.fn(), | ||
| back: jest.fn(), | ||
| }), | ||
| usePathname: () => '/', | ||
| useSearchParams: () => new URLSearchParams(), | ||
| })); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Stabilize router mocks so tests can assert push/replace calls
Each useRouter() call returns fresh jest.fn instances, which makes it hard to assert calls from the component under test. Provide stable fns and clear them between tests.
-// Mock next/navigation
-jest.mock('next/navigation', () => ({
- useRouter: () => ({
- push: jest.fn(),
- replace: jest.fn(),
- back: jest.fn(),
- }),
- usePathname: () => '/',
- useSearchParams: () => new URLSearchParams(),
-}));
+// Mock next/navigation with stable router fns shared across calls
+const __router = {
+ push: jest.fn(),
+ replace: jest.fn(),
+ back: jest.fn(),
+};
+jest.mock('next/navigation', () => ({
+ useRouter: () => __router,
+ usePathname: () => '/',
+ useSearchParams: () => new URLSearchParams(),
+}));
+
+afterEach(() => {
+ __router.push.mockClear();
+ __router.replace.mockClear();
+ __router.back.mockClear();
+});📝 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.
| // Mock next/navigation | |
| jest.mock('next/navigation', () => ({ | |
| useRouter: () => ({ | |
| push: jest.fn(), | |
| replace: jest.fn(), | |
| back: jest.fn(), | |
| }), | |
| usePathname: () => '/', | |
| useSearchParams: () => new URLSearchParams(), | |
| })); | |
| // Mock next/navigation with stable router fns shared across calls | |
| const __router = { | |
| push: jest.fn(), | |
| replace: jest.fn(), | |
| back: jest.fn(), | |
| }; | |
| jest.mock('next/navigation', () => ({ | |
| useRouter: () => __router, | |
| usePathname: () => '/', | |
| useSearchParams: () => new URLSearchParams(), | |
| })); | |
| afterEach(() => { | |
| __router.push.mockClear(); | |
| __router.replace.mockClear(); | |
| __router.back.mockClear(); | |
| }); |
🤖 Prompt for AI Agents
In jest.setup.js around lines 15 to 24, the next/navigation mock currently
returns new jest.fn instances on every useRouter() call which prevents tests
from asserting push/replace/back invocations; fix by declaring stable jest.fn
variables (e.g., push, replace, back) outside the mock and have useRouter return
those same function instances, and add a global beforeEach (or afterEach) in
this setup file to call .mockClear() on each of those stable fns so tests start
with a clean slate.
| "@testing-library/jest-dom": "^6.7.0", | ||
| "@testing-library/react": "^16.3.0", | ||
| "@testing-library/user-event": "^14.6.1", | ||
| "@types/jest": "^30.0.0", | ||
| "@types/node": "^20", |
There was a problem hiding this comment.
Tests will fail without transform deps: add babel-jest and @babel/core
Your jest.config.js uses the Babel transform with the Next preset, but devDependencies don’t include babel-jest or @babel/core. Without these, Jest cannot transform TS/TSX and will error at runtime.
"@testing-library/user-event": "^14.6.1",
"@types/jest": "^30.0.0",
+ "@babel/core": "^7.25.0",
"@types/node": "^20",
@@
"eslint-config-next": "15.3.2",
+ "babel-jest": "^29.7.0",
"jest": "^29.7.0",
"jest-environment-jsdom": "^29.7.0",Also applies to: 80-81
🤖 Prompt for AI Agents
In package.json around lines 70-74 (and also lines 80-81) the project is missing
transform dependencies required by the Jest Babel transform; add babel-jest and
@babel/core to devDependencies (matching compatible versions with your Jest/Next
setup), run npm/yarn install, and ensure package.json devDependencies list
includes both "babel-jest" and "@babel/core" so Jest can transform TS/TSX via
the Next/Babel preset.
| jest.mock('next/navigation', () => ({ | ||
| useRouter: () => ({ | ||
| push: jest.fn(), | ||
| replace: jest.fn(), | ||
| back: jest.fn(), | ||
| }), | ||
| usePathname: () => '/', | ||
| useSearchParams: () => new URLSearchParams(), | ||
| })); |
There was a problem hiding this comment.
Fix router mock: use a shared push mock; current assertion targets a different instance
useRouter() returns a new object with fresh jest.fn()s on every call. The component’s call and your test’s require('next/navigation').useRouter().push are different instances, so the navigation assertion can be false-negative/flaky. Hoist stable mocks and have useRouter return them.
Apply this diff:
import userEvent from '@testing-library/user-event';
-// Mock next/navigation
-jest.mock('next/navigation', () => ({
- useRouter: () => ({
- push: jest.fn(),
- replace: jest.fn(),
- back: jest.fn(),
- }),
- usePathname: () => '/',
- useSearchParams: () => new URLSearchParams(),
-}));
+// Stable router mocks shared between component and test assertions
+const pushMock = jest.fn();
+const replaceMock = jest.fn();
+const backMock = jest.fn();
+// Mock next/navigation
+jest.mock('next/navigation', () => ({
+ useRouter: jest.fn(() => ({
+ push: pushMock,
+ replace: replaceMock,
+ back: backMock,
+ })),
+ usePathname: jest.fn(() => '/'),
+ useSearchParams: jest.fn(() => new URLSearchParams()),
+}));🤖 Prompt for AI Agents
In src/__tests__/sign-in-view.test.tsx around lines 8 to 16, the jest.mock for
'next/navigation' creates fresh jest.fn() instances on each call causing
assertions to target a different push mock; replace it by hoisting shared mock
functions (e.g., const push = jest.fn(), replace = jest.fn(), back = jest.fn())
and have useRouter return that shared object so all calls reference the same
push mock, ensuring your test asserts against the same instance; also export or
expose those shared mocks if the test needs to reset or inspect them.
| //check if we are redirected to home | ||
| expect(require('next/navigation').useRouter().push).toHaveBeenCalledWith('/'); | ||
|
|
||
| }); |
There was a problem hiding this comment.
Assert the shared pushMock instead of calling useRouter() again
With the shared mock in place, assert against it directly. This avoids creating a fresh unused router instance.
- //check if we are redirected to home
- expect(require('next/navigation').useRouter().push).toHaveBeenCalledWith('/');
+ //check if we are redirected to home
+ expect(pushMock).toHaveBeenCalledWith('/');Optional hardening for future async changes to the auth flow:
- expect(pushMock).toHaveBeenCalledWith('/');
+ await waitFor(() => expect(pushMock).toHaveBeenCalledWith('/'));If you adopt the optional version, also update the import:
-import { render, screen } from '@testing-library/react';
+import { render, screen, waitFor } from '@testing-library/react';📝 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.
| //check if we are redirected to home | |
| expect(require('next/navigation').useRouter().push).toHaveBeenCalledWith('/'); | |
| }); | |
| // at the top of src/__tests__/sign-in-view.test.tsx, update your import: | |
| import { render, screen, waitFor } from '@testing-library/react'; | |
| // …later, in your test (around lines 50–53): | |
| // check if we are redirected to home | |
| // for async flows, wait for the navigation to have been triggered: | |
| await waitFor(() => expect(pushMock).toHaveBeenCalledWith('/')); | |
| }); |
🤖 Prompt for AI Agents
In src/__tests__/sign-in-view.test.tsx around lines 50 to 53, the test
re-invokes require('next/navigation').useRouter() to assert push was called
instead of asserting the shared pushMock; replace the
expect(require(...).useRouter().push).toHaveBeenCalledWith('/') with
expect(pushMock).toHaveBeenCalledWith('/') so the test asserts the existing mock
instance directly; optionally, if you want to harden for async auth flows wrap
the assertion in a waitFor (and import waitFor from '@testing-library/react') or
use await expect(() => expect(pushMock).toHaveBeenCalledWith('/')).not.toThrow()
and ensure any required imports are added.
1 participant
Summary by CodeRabbit
Tests
Chores
Documentation