[FEAT] - tests : set up Jest + React Testing Library with foundational test suite#210
Open
YashvardhanJani wants to merge 6 commits into
Open
[FEAT] - tests : set up Jest + React Testing Library with foundational test suite#210YashvardhanJani wants to merge 6 commits into
YashvardhanJani wants to merge 6 commits into
Conversation
|
@YashvardhanJani is attempting to deploy a commit to the vishnukothakapu's projects Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
|
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:
📝 WalkthroughWalkthroughAdds Jest + React Testing Library: config and setup files, npm scripts and devDependencies, TESTING.md, a GitHub Actions workflow, and comprehensive Jest + RTL tests for libs, middleware, components, and pages. ChangesTesting Infrastructure and Configuration
Library and Middleware Unit Tests
React Component Tests
Page Component Tests
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
19 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (7)
__tests__/components/DashboardNavbar.test.tsx-139-152 (1)
139-152:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winActive-route test currently can’t fail on regressions.
The assertion allows both presence and absence of active state, so route-highlighting changes won’t be caught.
Proposed fix
it("marks the dashboard link active when on /dashboard", () => { mockUseSession.mockReturnValue(authenticatedSession); mockUsePathname.mockReturnValue("/dashboard"); render(<DashboardNavbar />); - const links = screen.getAllByRole("link"); - const activeLink = links.find( - (l) => - l.getAttribute("aria-current") === "page" || - l.className.includes("active") - ); - expect(links.length).toBeGreaterThan(0); - // Active link may or may not exist depending on implementation - expect(activeLink === undefined || activeLink !== undefined).toBe(true); + const dashboardLink = screen.getAllByRole("link").find((l) => + l.getAttribute("href")?.includes("/dashboard") + ); + expect(dashboardLink).toBeDefined(); + expect( + dashboardLink?.getAttribute("aria-current") === "page" || + dashboardLink?.className.includes("active") + ).toBe(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 `@__tests__/components/DashboardNavbar.test.tsx` around lines 139 - 152, The test "marks the dashboard link active when on /dashboard" currently allows both presence and absence of an active link which prevents regressions from being caught; update the test to assert that an active link is present when mockUsePathname.mockReturnValue("/dashboard") is set: after rendering DashboardNavbar (using mockUseSession/authenticatedSession and mockUsePathname), find the link whose aria-current === "page" or whose className includes "active" and change the expectation to expect(activeLink).not.toBeUndefined() (or expect(activeLink).toBeTruthy()) so the test fails if no active route highlighting exists.__tests__/components/Navbar.test.tsx-144-152 (1)
144-152:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTheme-toggle test is a tautology.
expect(themeBtn === null || themeBtn !== null).toBe(true)always passes and doesn’t validate anything.Proposed fix
it("renders the theme toggle button", () => { renderNavbar(); - const themeBtn = screen.queryByRole("button", { - name: /theme|dark|light|toggle/i, - }); - // It's optional in Navbar vs DashboardNavbar, so we just check it doesn't throw - expect(themeBtn === null || themeBtn !== null).toBe(true); + expect( + screen.getByRole("button", { name: /theme|dark|light|toggle/i }) + ).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 `@__tests__/components/Navbar.test.tsx` around lines 144 - 152, The test "renders the theme toggle button" currently contains a tautology; update it so it actually validates behavior: either assert the toggle exists by using screen.getByRole("button", { name: /theme|dark|light|toggle/i }) and expect(...) toBeInTheDocument() if Navbar should include the control, or (if the toggle is optional) replace the tautological expect with a real non-throwing or type check such as asserting that calling screen.queryByRole(...) does not throw or that the returned themeBtn is either null or an HTMLButtonElement (e.g. expect(themeBtn === null || themeBtn instanceof HTMLButtonElement).toBe(true)); locate the test by the "renders the theme toggle button" it block and the renderNavbar / themeBtn / queryByRole symbols to apply the change.__tests__/components/DashboardNavbar.test.tsx-93-99 (1)
93-99:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReplace tautological email assertion with a real visibility/disclosure check.
expect(emailEl === null || emailEl !== null).toBe(true)always passes, so this test never validates behavior.Proposed fix
it("displays the user's email", () => { mockUseSession.mockReturnValue(authenticatedSession); render(<DashboardNavbar />); - const emailEl = screen.queryByText(/vishnu@example\.com/i); - // Email may be in a dropdown — acceptable if not immediately visible - expect(emailEl === null || emailEl !== null).toBe(true); + // If email is in collapsed menu, open it first and then assert. + const trigger = screen.getByRole("button"); + fireEvent.click(trigger); + expect(screen.getByText(/vishnu@example\.com/i)).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 `@__tests__/components/DashboardNavbar.test.tsx` around lines 93 - 99, The test currently contains a tautological assertion and should instead verify actual visibility/disclosure of the email: update the DashboardNavbar test (where mockUseSession returns authenticatedSession and emailEl = screen.queryByText(/vishnu@example\.com/i)) to assert real behavior — if emailEl is non-null assert it is visible (e.g., toBeVisible), otherwise locate the navbar user menu toggle (e.g., the role/button that opens the user dropdown in DashboardNavbar), click it, then assert the email is present in the DOM (getByText or queryByText) and visible; ensure you use the existing mocks (mockUseSession, authenticatedSession) and the DashboardNavbar component identifiers to locate the menu toggle and email element.__tests__/components/Navbar.test.tsx-159-175 (1)
159-175:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winActive-route test currently has no effective assertion.
expect(activeLinks.length >= 0).toBe(true)is always true. Also, this component styles active state via section class logic rather than guaranteedaria-current.Proposed fix
it("applies an active class or aria-current to the current page link", () => { ... const activeLinks = links.filter( (el) => el.getAttribute("aria-current") === "page" || el.className.includes("active") ); - - expect(links.length).toBeGreaterThan(0); - expect(activeLinks.length >= 0).toBe(true); + expect(links.length).toBeGreaterThan(0); + expect(activeLinks.length).toBeGreaterThan(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 `@__tests__/components/Navbar.test.tsx` around lines 159 - 175, The current test assertion is ineffective because expect(activeLinks.length >= 0).toBe(true) is always true; update the assertion in the "applies an active class or aria-current to the current page link" test (which uses mockUseSession, mockUsePathname, renderNavbar and screen.getAllByRole) to actually verify active state: either assert expect(activeLinks.length).toBeGreaterThan(0) or, more robustly, locate the link corresponding to the mocked pathname (e.g., find the link with href "/login" among the results from screen.getAllByRole) and assert that that element has aria-current === "page" or its className includes "active".TESTING.md-27-36 (1)
27-36:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a language identifier to the fenced code block.
Line 27 should include a language token to satisfy markdown linting.
Suggested fix
-``` +```text __tests__/ ├── lib/ │ ├── platforms.test.ts ← Platform URL detection & validation │ └── url.test.ts ← URL helper utility functions ├── components/ │ └── Navbar.test.tsx ← Navbar smoke + behaviour tests └── middleware/ └── csrf.test.ts ← CSRF middleware tests</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@TESTING.mdaround lines 27 - 36, The fenced code block showing the
tests/ directory tree (the block starting withand containing platforms.test.ts, url.test.ts, Navbar.test.tsx, csrf.test.ts) lacks a language identifier; update the opening fence to include a language token (e.g.,text)
so the markdown linter passes, keeping the block content unchanged.</details> </blockquote></details> <details> <summary>__tests__/components/ProfileCard.test.tsx-177-185 (1)</summary><blockquote> `177-185`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Test name and assertion are inconsistent for external-link rel security.** The test says `rel=noopener noreferrer`, but only `noopener` is validated. Add `noreferrer` assertion to match the stated contract. <details> <summary>Suggested fix</summary> ```diff externalLinks.forEach((link) => { const rel = link.getAttribute("rel") ?? ""; expect(rel).toContain("noopener"); + expect(rel).toContain("noreferrer"); }); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@__tests__/components/ProfileCard.test.tsx` around lines 177 - 185, The test "external links have rel=noopener noreferrer for security" in __tests__/components/ProfileCard.test.tsx currently only asserts that rel contains "noopener"; update the assertion to also check for "noreferrer" so the test matches its name/contract. Locate the test block (variables links, externalLinks) and add an assertion such as expecting the rel string to contain "noreferrer" for each external link in the externalLinks.forEach loop. ``` </details> </blockquote></details> <details> <summary>__tests__/pages/LoginPage.test.tsx-211-215 (1)</summary><blockquote> `211-215`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Fix email “associated label” assertion in LoginPage accessibility test** `__tests__/pages/LoginPage.test.tsx` falls back to `document.querySelector('input[type="email"]')`, so the test can pass even if the email input has no associated `<label>`/`aria-label`. <details> <summary>Suggested fix</summary> ```diff -const emailInput = - screen.queryByLabelText(/email/i) || - document.querySelector('input[type="email"]'); -expect(emailInput).toBeInTheDocument(); +expect(screen.getByLabelText(/email/i)).toBeInTheDocument(); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@__tests__/pages/LoginPage.test.tsx` around lines 211 - 215, The test "email input has an associated label" currently falls back to document.querySelector('input[type="email"]') which lets the test pass without a real label; remove that fallback and assert the label association directly by using the label query (e.g., replace the queryByLabelText + fallback with a direct screen.getByLabelText(/email/i) or expect(screen.queryByLabelText(/email/i)).toBeInTheDocument()) so the test fails when no <label> or aria-label is present; keep the test name and replace references to document.querySelector('input[type="email"]') and the variable emailInput accordingly. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (2)</summary><blockquote> <details> <summary>__tests__/lib/platforms.test.ts (1)</summary><blockquote> `20-34`: _⚡ Quick win_ **Type the local platform list to prevent silent drift from `Platform` union.** Use `Exclude<Platform, "website">[]` so typos/invalid entries fail at compile time. <details> <summary>Proposed refactor</summary> ```diff -const SUPPORTED_PLATFORMS = [ +const SUPPORTED_PLATFORMS: Exclude<Platform, "website">[] = [ "github", "linkedin", "leetcode", "youtube", "x", "instagram", "facebook", "discord", "twitch", "hashnode", "devto", "medium", "dribbble", ]; ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@__tests__/lib/platforms.test.ts` around lines 20 - 34, The SUPPORTED_PLATFORMS array should be strongly typed to prevent drifting from the Platform union: change its declaration to have the type Exclude<Platform, "website">[] so any typo or invalid platform entries (e.g., in SUPPORTED_PLATFORMS) fail at compile time; update the const SUPPORTED_PLATFORMS declaration to use that type and keep the same string entries unchanged. ``` </details> </blockquote></details> <details> <summary>__tests__/pages/RegisterPage.test.tsx (1)</summary><blockquote> `119-124`: _⚡ Quick win_ **Toggle selection by array index is brittle.** Using `allToggleBtns[0]` / `[1]` (Line 123, Line 173, Line 199) couples tests to DOM order. A markup reorder can break intent or assert against the wrong field. Prefer selecting each toggle relative to its input/container (e.g., via `within(...)` on the field wrapper) or by distinct accessible name. Also applies to: 172-174, 199-205 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@__tests__/pages/RegisterPage.test.tsx` around lines 119 - 124, The test currently picks toggles by array index (allToggleBtns[0], [1], etc.), which is brittle; update the selection to find each toggle relative to its corresponding input/container instead — for example locate the password input with screen.getByLabelText or screen.getByPlaceholderText, use within(passwordField.closest or within(containerElement)) or within(fieldWrapper) and call getByRole('button', { name: /show|hide|eye/i }) to get that field's toggle; apply the same change to every place using allToggleBtns (variables allToggleBtns and toggleBtn) so each toggle is tied to its specific input rather than DOM order. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>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@__tests__/components/DashboardNavbar.test.tsx:
- Around line 179-188: The test currently uses a permissive query and an if
(signOutBtn) guard so it can pass without the sign-out control being rendered;
change the test to require the control and exercise the dropdown before
clicking: use a throwing query (getByRole/getByText) instead of
queryByRole/queryByText to assert the sign-out button exists, remove the if
guard, and—if the sign-out is inside a menu—first open the menu by clicking the
navbar menu toggle (the element that opens the user dropdown in DashboardNavbar)
before locating and clicking the sign-out control; finally assert mockSignOut
was called once (mockSignOut) so the test fails if the UI path is missing.In
@__tests__/components/Navbar.test.tsx:
- Around line 78-133: The tests assume Navbar conditionally shows
Dashboard/sign-in/user identity based on useSession, but Navbar (component
Navbar) currently renders a static nav including "Get Started" and does not
branch on useSession; update the test file
(tests/components/Navbar.test.tsx) to stop asserting session-driven
behavior: adjust or remove the authenticated/unauthenticated describe blocks
that check for Dashboard, Sign In, and user name/avatar, and instead assert the
static elements Navbar actually renders (e.g., site links, "Get Started") using
the existing renderNavbar helper and mockUsePathname/mockUseSession only as
needed for pathname-dependent behavior; alternatively, if you prefer
behavior-based tests, modify the Navbar component to implement conditional
rendering using useSession so the tests' expectations match (update Navbar to
check useSession and render Dashboard/sign-in/user identity accordingly).In
@__tests__/components/ProfileCard.test.tsx:
- Around line 195-200: The current assertion in
tests/components/ProfileCard.test.tsx uses a tautology (expect(emptyMsg ===
null || emptyMsg !== null).toBe(true)) which never fails; replace it with a
single explicit assertion that reflects the intended empty-state behavior:
either assert that emptyMsg is null (expect(emptyMsg).toBeNull()) if the
component should render nothing, or assert that the empty-state message exists
(expect(emptyMsg).not.toBeNull() or expect(screen.getByText(/no links|nothing
here|add your first link/i)).toBeInTheDocument()) if it should show text. Update
the test around the emptyMsg variable and queryByText call to use that one
explicit expectation.In
@__tests__/components/ThemeToggle.test.tsx:
- Around line 152-153: The assertions using "expect(lightEl === null || lightEl
!== null).toBe(true)" (and the similar tautology for darkEl) don't test
anything; instead, update the ThemeToggle.test.tsx to drive observable behavior:
render the ThemeToggle, use the toggle/menu trigger (e.g., the element queried
as toggleButton or getByRole('button')), fireEvent or userEvent to open the
options, then assert real visibility/interactivity of lightEl and darkEl (use
getByText/getByRole or queryByText and
expect(...).toBeInTheDocument()/toBeVisible()/toHaveAttribute(...) or simulate
clicking an option and assert the theme change). Replace the tautological
expects with these interaction-based assertions referencing the existing
variables lightEl and darkEl and the toggle/menu trigger used in the test.- Around line 74-82: Replace the conditional assertion pattern that uses
screen.queryByRole + if (btn) with a failing lookup using screen.getByRole so
the test fails if the button is missing; e.g., in the blocks that call
screen.queryByRole("button") and then guard with if (btn) before computing label
from btn.getAttribute("aria-label") || btn.getAttribute("title") ||
btn.textContent and expect(label).toBeTruthy(), switch to
screen.getByRole("button") (assign to btn) and remove the if guard so the expect
always runs. Apply the same change to all similar occurrences (the blocks around
the ranges mentioned) so assertions are not silently skipped.In
@__tests__/lib/platforms.test.ts:
- Around line 60-76: The test fixtures contain URLs that don't match the
project's PLATFORM_PATTERNS: update the x fixture entries in the x array to only
use accepted X domain forms (e.g., replace "https://twitter.com/vishnu" with an
equivalent "https://x.com/..." or "https://www.x.com/..."), replace the discord
entry "https://discord.gg/inviteCode" in the discord array with a URL matching
the discord regex (e.g., a discord.com invite or user URL that PLATFORM_PATTERNS
accepts) or alternatively broaden the discord pattern in PLATFORM_PATTERNS if
you intend to accept .gg invites, and remove or move the "github.com/user"
fixture out of invalidUrls (since github.com/user is normalized and valid under
PLATFORM_PATTERNS). Ensure changes reference the x array, discord array,
invalidUrls, and PLATFORM_PATTERNS so the test fixtures align with the regex
expectations.- Around line 110-132: The tests incorrectly expect null from detectPlatform()
when no known platform matches; per lib/platforms.ts detectPlatform returns
"website" as a fallback. Update assertions in these cases to expect "website"
instead of null (e.g. change expect(detectPlatform("")).toBeNull() and other
similar asserts to expect(detectPlatform(...)).toBe("website")), and adjust the
case-insensitive test to accept either "github" or "website" (e.g.
expect(["github","website"]).toContain(result)). Ensure you only modify the
assertions referencing detectPlatform in this test block.In
@__tests__/lib/url.test.ts:
- Around line 25-35: The test's invalid fixture array conflicts with the current
isValidHttpUrl() behavior: entries "not-a-url" and "github.com/user" are treated
as valid because missing schemes are normalized; update the test to match
behavior by removing or relocating those two strings from the invalid array (the
invalid constant in tests/lib/url.test.ts) into the valid fixtures, or
adjust expectations to assert normalization via isValidHttpUrl() and any
normalization function used by isValidHttpUrl; ensure references to
isValidHttpUrl and the invalid array are updated so tests reflect actual URL
normalization behavior.In
@__tests__/middleware/csrf.test.ts:
- Around line 50-59: The test for unsafeMethods currently permits null/other
non-403 outcomes which can hide regressions; update the test using buildRequest
and applyCsrfProtection so that for each unsafe method (POST|PUT|PATCH|DELETE)
to "https://linkid.qzz.io/api/links" the result is asserted to be an instance of
NextResponse with status 403 (i.e., remove the branch allowing null/>=400 and
instead expect result instanceof NextResponse and
expect(result.status).toBe(403)), ensuring the middleware deterministically
rejects missing CSRF tokens per lib/middleware/csrf.ts.- Around line 66-85: Tests claiming "origin validation" are misleading because
applyCsrfProtection() currently doesn’t check the Origin header; either update
the middleware or fix the tests. Option A (preferred): implement origin
validation inside applyCsrfProtection (in lib/middleware/csrf.ts) by comparing
the request.headers.get('Origin') to the request.url origin and returning a
rejecting NextResponse (4xx) when they differ; keep existing token handling.
Option B: if you don’t want origin checks, remove or change the two "origin
validation" test cases in tests/middleware/csrf.test.ts to assert only the
actual contract (CSRF token behavior) and not Origin-based blocking; reference
applyCsrfProtection and the tests to locate the relevant code paths.- Line 94: The test uses an invalid matcher: applyCsrfProtection(req) resolves
to NextResponse | null but the code calls .resolves.not.toThrow(), which expects
a function; change the assertion to assert the resolved value directly (e.g.,
await expect(applyCsrfProtection(req)).resolves.toBeNull() for the pass-through
case). Locate the assertion using applyCsrfProtection in the test and replace
.resolves.not.toThrow() with the appropriate value assertion
(resolves.toBeNull() or resolves.toBeInstanceOf(NextResponse) depending on the
specific case being tested).In @.github/workflows/jest.yml:
- Around line 9-13: Add an explicit least-privilege permissions block to the
workflow (e.g., top-level permissions: contents: read) to restrict GITHUB_TOKEN
scope, update the checkout step to set persist-credentials: false, and pin the
referenced actions (actions/checkout@v4, actions/setup-node@v4,
actions/upload-artifact@v4) to their specific commit SHAs instead of floating
tags to ensure immutability and supply provenance; specifically, modify the
jobs.test workflow to include the top-level permissions entry, change the
checkout step to include persist-credentials: false, and replace the three uses:
lines with the corresponding stable SHA pins for actions/checkout,
actions/setup-node, and actions/upload-artifact.- Around line 15-37: The workflow uses mutable action tags and leaves checkout
credentials persistent; update the GitHub Actions steps that reference
actions/checkout@v4, actions/setup-node@v4, and actions/upload-artifact@v4 to
pinned commit SHAs (replace the tag refs with the corresponding full commit SHAs
for each action) and modify the checkout step (symbol: actions/checkout) to
include persist-credentials: false to disable credential persistence; keep the
existing inputs (node-version/cache/env) unchanged while only replacing the
action refs and adding the persist-credentials flag.In
@jest.setup.ts:
- Around line 55-69: The current beforeAll override for console.error (using
originalConsoleError and beforeAll) broadly suppresses any message containing
"Warning:", which can hide real test issues; update the filter to remove the
generic msg.includes("Warning:") check and instead list only the explicit noisy
messages to ignore (e.g., exact React 19/RTL noise strings you observed) by
matching full substrings or regexes for those known cases inside the
console.error override, keeping originalConsoleError(...args) for everything
else so legitimate warnings still surface; ensure the function names referenced
are console.error, originalConsoleError and the beforeAll hook so you change the
predicate there.- Around line 13-26: In jest.setup.ts make the mocked next/navigation exports
real Jest mocks so tests can override them: change useRouter, usePathname and
useSearchParams to return jest.fn() mocks (e.g., export usePathname: jest.fn(()
=> "/") and useSearchParams: jest.fn(() => new URLSearchParams())) so tests that
cast to jest.Mock can call mockReturnValue; also tighten the console.error
override by filtering only the specific expected warning message substring (not
every message containing "Warning:") to avoid suppressing unrelated console
errors.
Minor comments:
In@__tests__/components/DashboardNavbar.test.tsx:
- Around line 139-152: The test "marks the dashboard link active when on
/dashboard" currently allows both presence and absence of an active link which
prevents regressions from being caught; update the test to assert that an active
link is present when mockUsePathname.mockReturnValue("/dashboard") is set: after
rendering DashboardNavbar (using mockUseSession/authenticatedSession and
mockUsePathname), find the link whose aria-current === "page" or whose className
includes "active" and change the expectation to
expect(activeLink).not.toBeUndefined() (or expect(activeLink).toBeTruthy()) so
the test fails if no active route highlighting exists.- Around line 93-99: The test currently contains a tautological assertion and
should instead verify actual visibility/disclosure of the email: update the
DashboardNavbar test (where mockUseSession returns authenticatedSession and
emailEl = screen.queryByText(/vishnu@example.com/i)) to assert real behavior —
if emailEl is non-null assert it is visible (e.g., toBeVisible), otherwise
locate the navbar user menu toggle (e.g., the role/button that opens the user
dropdown in DashboardNavbar), click it, then assert the email is present in the
DOM (getByText or queryByText) and visible; ensure you use the existing mocks
(mockUseSession, authenticatedSession) and the DashboardNavbar component
identifiers to locate the menu toggle and email element.In
@__tests__/components/Navbar.test.tsx:
- Around line 144-152: The test "renders the theme toggle button" currently
contains a tautology; update it so it actually validates behavior: either assert
the toggle exists by using screen.getByRole("button", { name:
/theme|dark|light|toggle/i }) and expect(...) toBeInTheDocument() if Navbar
should include the control, or (if the toggle is optional) replace the
tautological expect with a real non-throwing or type check such as asserting
that calling screen.queryByRole(...) does not throw or that the returned
themeBtn is either null or an HTMLButtonElement (e.g. expect(themeBtn === null
|| themeBtn instanceof HTMLButtonElement).toBe(true)); locate the test by the
"renders the theme toggle button" it block and the renderNavbar / themeBtn /
queryByRole symbols to apply the change.- Around line 159-175: The current test assertion is ineffective because
expect(activeLinks.length >= 0).toBe(true) is always true; update the assertion
in the "applies an active class or aria-current to the current page link" test
(which uses mockUseSession, mockUsePathname, renderNavbar and
screen.getAllByRole) to actually verify active state: either assert
expect(activeLinks.length).toBeGreaterThan(0) or, more robustly, locate the link
corresponding to the mocked pathname (e.g., find the link with href "/login"
among the results from screen.getAllByRole) and assert that that element has
aria-current === "page" or its className includes "active".In
@__tests__/components/ProfileCard.test.tsx:
- Around line 177-185: The test "external links have rel=noopener noreferrer for
security" in tests/components/ProfileCard.test.tsx currently only asserts
that rel contains "noopener"; update the assertion to also check for
"noreferrer" so the test matches its name/contract. Locate the test block
(variables links, externalLinks) and add an assertion such as expecting the rel
string to contain "noreferrer" for each external link in the
externalLinks.forEach loop.In
@__tests__/pages/LoginPage.test.tsx:
- Around line 211-215: The test "email input has an associated label" currently
falls back to document.querySelector('input[type="email"]') which lets the test
pass without a real label; remove that fallback and assert the label association
directly by using the label query (e.g., replace the queryByLabelText + fallback
with a direct screen.getByLabelText(/email/i) or
expect(screen.queryByLabelText(/email/i)).toBeInTheDocument()) so the test fails
when no or aria-label is present; keep the test name and replace
references to document.querySelector('input[type="email"]') and the variable
emailInput accordingly.In
@TESTING.md:
- Around line 27-36: The fenced code block showing the tests/ directory tree
(the block starting withand containing platforms.test.ts, url.test.ts, Navbar.test.tsx, csrf.test.ts) lacks a language identifier; update the opening fence to include a language token (e.g.,text) so the markdown linter passes,
keeping the block content unchanged.
Nitpick comments:
In@__tests__/lib/platforms.test.ts:
- Around line 20-34: The SUPPORTED_PLATFORMS array should be strongly typed to
prevent drifting from the Platform union: change its declaration to have the
type Exclude<Platform, "website">[] so any typo or invalid platform entries
(e.g., in SUPPORTED_PLATFORMS) fail at compile time; update the const
SUPPORTED_PLATFORMS declaration to use that type and keep the same string
entries unchanged.In
@__tests__/pages/RegisterPage.test.tsx:
- Around line 119-124: The test currently picks toggles by array index
(allToggleBtns[0], [1], etc.), which is brittle; update the selection to find
each toggle relative to its corresponding input/container instead — for example
locate the password input with screen.getByLabelText or
screen.getByPlaceholderText, use within(passwordField.closest or
within(containerElement)) or within(fieldWrapper) and call getByRole('button', {
name: /show|hide|eye/i }) to get that field's toggle; apply the same change to
every place using allToggleBtns (variables allToggleBtns and toggleBtn) so each
toggle is tied to its specific input rather than DOM order.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro Plus **Run ID**: `9c6e3bcc-4563-404f-8afc-7a4f9b0eed34` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between fcabba852c55407dc62a527bb93283824ae839db and bcd589fa01db01773798cccadd85f535c04fecd0. </details> <details> <summary>⛔ Files ignored due to path filters (1)</summary> * `package-lock.json` is excluded by `!**/package-lock.json` </details> <details> <summary>📒 Files selected for processing (14)</summary> * `.github/workflows/jest.yml` * `TESTING.md` * `__tests__/components/DashboardNavbar.test.tsx` * `__tests__/components/Navbar.test.tsx` * `__tests__/components/ProfileCard.test.tsx` * `__tests__/components/ThemeToggle.test.tsx` * `__tests__/lib/platforms.test.ts` * `__tests__/lib/url.test.ts` * `__tests__/middleware/csrf.test.ts` * `__tests__/pages/LoginPage.test.tsx` * `__tests__/pages/RegisterPage.test.tsx` * `jest.config.ts` * `jest.setup.ts` * `package.json` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Owner
|
Thanks for the PR @YashvardhanJani. Could you please resolve the above CodeRabbit review comments and mark them as resolved after making the required changes? |
Anushreebasics
requested changes
May 24, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
__tests__/components/DashboardNavbar.test.tsx (2)
93-99:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEmail test is a tautology and never validates behavior
emailEl === null || emailEl !== nullis always true, so this test passes regardless of UI output.Suggested fix
it("displays the user's email", () => { mockUseSession.mockReturnValue(authenticatedSession); render(<DashboardNavbar />); - const emailEl = screen.queryByText(/vishnu@example\.com/i); - // Email may be in a dropdown — acceptable if not immediately visible - expect(emailEl === null || emailEl !== null).toBe(true); + fireEvent.click(screen.getByRole("button", { name: /vishnu|account|menu/i })); + expect(screen.getByText(/vishnu@example\.com/i)).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 `@__tests__/components/DashboardNavbar.test.tsx` around lines 93 - 99, The test "displays the user's email" currently contains a tautological assertion; replace the always-true check with a real assertion that validates UI behavior for DashboardNavbar: when mockUseSession is set to authenticatedSession, assert that the user's email (vishnu@example.com) is rendered by using a presence check (e.g., expect(screen.getByText(/vishnu@example\.com/i)).toBeInTheDocument()) or, if the email may be inside a closed dropdown, simulate opening the dropdown control (find the dropdown toggle in DashboardNavbar) and then assert the email is present using screen.getByText or await findByText; reference mockUseSession, authenticatedSession, and DashboardNavbar to locate and update the test.
139-158:⚠️ Potential issue | 🟠 Major | ⚡ Quick winActive-route tests currently don’t assert active-route behavior
Both cases can pass without any active-state rendering (
truetautology and render-without-throw), so regressions in highlighting won’t be caught.Suggested fix
it("marks the dashboard link active when on /dashboard", () => { mockUseSession.mockReturnValue(authenticatedSession); mockUsePathname.mockReturnValue("/dashboard"); render(<DashboardNavbar />); - const links = screen.getAllByRole("link"); - const activeLink = links.find( - (l) => - l.getAttribute("aria-current") === "page" || - l.className.includes("active") - ); - expect(links.length).toBeGreaterThan(0); - // Active link may or may not exist depending on implementation - expect(activeLink === undefined || activeLink !== undefined).toBe(true); + const dashboardLink = screen.getByRole("link", { name: /dashboard/i }); + expect( + dashboardLink.getAttribute("aria-current") === "page" || + dashboardLink.className.includes("active") + ).toBe(true); }); it("marks the profile link active when on /profile", () => { mockUseSession.mockReturnValue(authenticatedSession); mockUsePathname.mockReturnValue("/profile"); - expect(() => render(<DashboardNavbar />)).not.toThrow(); + render(<DashboardNavbar />); + const profileLink = screen.getByRole("link", { name: /profile/i }); + expect( + profileLink.getAttribute("aria-current") === "page" || + profileLink.className.includes("active") + ).toBe(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 `@__tests__/components/DashboardNavbar.test.tsx` around lines 139 - 158, The tests currently don't verify active-route highlighting; update the two specs for DashboardNavbar to assert the actual active state: use mockUsePathname to return "/dashboard" and "/profile" respectively, render(<DashboardNavbar />), then locate the specific nav link by its accessible text (e.g., find the "Dashboard" and "Profile" link via screen.getByRole("link", { name: /Dashboard/i }) / getByRole(... "Profile")) and assert that the link has aria-current="page" or its className includes "active" (or both) instead of the tautological checks; keep mockUseSession/mockUsePathname and authenticatedSession setup but replace the no-op assertions with explicit active-state expectations so regressions in DashboardNavbar active highlighting fail the tests.__tests__/pages/LoginPage.test.tsx (1)
118-129:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSubmit-button test is selecting the wrong control and asserting the wrong initial state.
Line 118 can match OAuth buttons (
Continue with ...), so the test may never validate the email-login submit button. Also, the initial-state assertion conflicts withdisabled={loading || isEmailAndPasswordEmpty()}inapp/login/page.tsx.Suggested fix
- it("renders a submit / sign in button", () => { - const submitBtn = - screen.queryByRole("button", { name: /sign in|log in|login|continue/i }) ?? - screen.queryByRole("button", { name: /submit/i }); - expect(submitBtn).toBeInTheDocument(); - }); + it("renders the email login submit button", () => { + const submitBtn = screen.getByRole("button", { name: /login with email|sign in|log in/i }); + expect(submitBtn).toBeInTheDocument(); + }); - it("submit button is not disabled initially", () => { - const submitBtn = screen.queryByRole("button", { - name: /sign in|log in|login|continue/i, - }); - if (submitBtn) { - expect(submitBtn).not.toBeDisabled(); - } - }); + it("submit button is disabled until email and password are entered", () => { + const submitBtn = screen.getByRole("button", { name: /login with email|sign in|log in/i }); + expect(submitBtn).toBeDisabled(); + });🤖 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 `@__tests__/pages/LoginPage.test.tsx` around lines 118 - 129, The test is selecting generic OAuth "Continue with..." buttons and asserting the wrong initial state; update the test to target the email-login submit control specifically and assert its disabled state matches the component logic in app/login/page.tsx (disabled={loading || isEmailAndPasswordEmpty()}). Locate the submit button selection in the test (the submitBtn variable) and change the query to scope to the email/password form or match a label/text unique to the email-login submit (instead of the broad /sign in|log in|login|continue/i), then assert expect(submitBtn).toBeDisabled() when fields are initially empty (or verify loading false + isEmailAndPasswordEmpty() behavior) so the test mirrors the isEmailAndPasswordEmpty() disabled logic.
♻️ Duplicate comments (3)
.github/workflows/jest.yml (1)
19-19:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winReplace placeholder action refs with real pinned SHAs.
uses: actions/*@<full-length-sha>is not a valid ref and will fail workflow resolution. Pin each action to an actual full commit SHA (40 hex chars).Suggested fix
- uses: actions/checkout@<full-length-sha> + uses: actions/checkout@<actual-40-char-commit-sha> @@ - uses: actions/setup-node@<full-length-sha> + uses: actions/setup-node@<actual-40-char-commit-sha> @@ - uses: actions/upload-artifact@<full-length-sha> + uses: actions/upload-artifact@<actual-40-char-commit-sha>Also applies to: 25-25, 42-42
🤖 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 @.github/workflows/jest.yml at line 19, The workflow contains invalid placeholder refs like "uses: actions/checkout@<full-length-sha>" (and the similar entries at the other occurrences) which will fail; replace each placeholder with an actual full 40-character commit SHA for the referenced action (e.g., actions/checkout@<40-hex-sha>) by looking up the target action repo's commit and pasting the full SHA into each "uses:" line so every action reference is pinned to a real commit.__tests__/components/ProfileCard.test.tsx (1)
193-200:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEmpty-state test has no executable assertion.
On Line 193, the test claims an empty-state contract but currently only contains commented alternatives, so it always passes without validating behavior.
Suggested minimal fix
it("shows an empty-state message when user has no links", () => { render(<ProfileCard {...baseProps} user={{ ...baseProps.user, links: [] }} />); - // Pick one contract and enforce it: - // 1) if empty copy is required: - // expect(screen.getByText(/add your first link/i)).toBeInTheDocument(); - // 2) if no empty copy is expected: - // expect(screen.queryByText(/no links|nothing here|add your first link/i)).toBeNull(); + expect(screen.queryByText(/no links|nothing here|add your first link/i)).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 `@__tests__/components/ProfileCard.test.tsx` around lines 193 - 200, The test "shows an empty-state message when user has no links" currently has no assertion; update the test after the render(<ProfileCard ... user={{ ...baseProps.user, links: [] }} />) call to enforce the intended contract: either assert that the empty-state copy is shown (e.g., use screen.getByText(/add your first link/i) and expect(...).toBeInTheDocument()) or assert that no empty copy appears (e.g., use screen.queryByText(/no links|nothing here|add your first link/i) and expect(...).toBeNull()); pick the correct contract for this component and add the corresponding expect statement so the test actually verifies behavior.__tests__/components/DashboardNavbar.test.tsx (1)
179-183:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSign-out flow still uses an ambiguous trigger button
Using
screen.getByRole("button")may click the wrong control (e.g., theme toggle), so this test can be flaky or fail for the wrong reason. Target the user-menu trigger explicitly before clicking sign out.Suggested fix
it("calls signOut() when the sign-out button is clicked", () => { render(<DashboardNavbar />); - fireEvent.click(screen.getByRole("button")); - const signOutBtn = screen.getByText(/sign out|logout|log out/i); + fireEvent.click(screen.getByRole("button", { name: /vishnu|account|menu/i })); + const signOutBtn = screen.getByRole("menuitem", { name: /sign out|logout|log out/i }); fireEvent.click(signOutBtn); expect(mockSignOut).toHaveBeenCalledWith({ callbackUrl: "/login" }); });🤖 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 `@__tests__/components/DashboardNavbar.test.tsx` around lines 179 - 183, The test clicks a generic button via screen.getByRole("button") which can hit the wrong control (e.g., theme toggle) and make the sign-out spec flaky; update the test in __tests__/components/DashboardNavbar.test.tsx to explicitly open the user-menu trigger before clicking the sign-out item—locate the user menu trigger used by DashboardNavbar (e.g., the avatar/profile button or the button with an accessible name like "open user menu" / "account") using a more specific query such as getByRole("button", { name: /user|account|open user menu|profile/i }) or a test id used by the component, click that to reveal the menu, then find the signOutBtn (screen.getByText(/sign out|logout|log out/i)) and click it.
🤖 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 `@__tests__/components/Navbar.test.tsx`:
- Around line 83-90: The test in __tests__/components/Navbar.test.tsx
incorrectly filters section links by href starting with "#" even though Navbar
emits section links as "/#section" (e.g. "/#features"); update the selector in
the test (the anchorLinks filter that inspects link.getAttribute("href")) to
check for "/#" (e.g., startsWith("/#") or includes("/#")) so the test matches
the component's actual href format when collecting links from
screen.getAllByRole("link").
In `@__tests__/middleware/csrf.test.ts`:
- Around line 78-79: The test currently allows either null or a NextResponse
which hides regressions; change the assertion in the csurf test so that when
calling applyCsrfProtection(req) for the excluded path (e.g.,
"/api/auth/session") the result must be null: replace the loose check
(expect(result === null || result instanceof NextResponse).toBe(true)) with a
deterministic assertion like expect(result).toBeNull(); keep the same request
setup and ensure you reference applyCsrfProtection and NextResponse in the test
so readers know this is the excluded-route behavior.
In `@__tests__/pages/LoginPage.test.tsx`:
- Around line 189-211: The tests "register link points to /register" and
"register link text is 'Sign up' (two words, screen-reader friendly)" currently
use screen.queryByRole and a regex that omits "Signup", plus they guard
assertions with if(registerLink) which can silently skip failures; change both
queries to screen.getByRole("link", { name: /sign up|signup|register|create
account/i }) so missing links throw, remove the conditional if(registerLink)
guards, and assert directly on the retrieved registerLink (e.g.,
expect(registerLink.getAttribute("href")).toContain("/register") and
expect(registerLink.textContent?.toLowerCase()).toMatch(/sign
up|signup|register|create account/)).
- Around line 223-225: The test currently falls back to a DOM query instead of
asserting a real accessible name; remove the "??
document.querySelector('input[type=\"email\"]')" fallback and change the
assertion to require an actual label/ARIA name (e.g., use
getByLabelText(/email/i) or
expect(screen.getByLabelText(/email/i)).toBeInTheDocument()). If the
LoginPage/Input component lacks an accessible name, add one in the component
under test (either a <label htmlFor="email">Email</label> with matching id on
the input or add aria-label="Email" / aria-labelledby) so the test can reliably
find the control by its accessible name; update the test to reference the same
pattern (getByLabelText) and remove the generic querySelector fallback.
---
Outside diff comments:
In `@__tests__/components/DashboardNavbar.test.tsx`:
- Around line 93-99: The test "displays the user's email" currently contains a
tautological assertion; replace the always-true check with a real assertion that
validates UI behavior for DashboardNavbar: when mockUseSession is set to
authenticatedSession, assert that the user's email (vishnu@example.com) is
rendered by using a presence check (e.g.,
expect(screen.getByText(/vishnu@example\.com/i)).toBeInTheDocument()) or, if the
email may be inside a closed dropdown, simulate opening the dropdown control
(find the dropdown toggle in DashboardNavbar) and then assert the email is
present using screen.getByText or await findByText; reference mockUseSession,
authenticatedSession, and DashboardNavbar to locate and update the test.
- Around line 139-158: The tests currently don't verify active-route
highlighting; update the two specs for DashboardNavbar to assert the actual
active state: use mockUsePathname to return "/dashboard" and "/profile"
respectively, render(<DashboardNavbar />), then locate the specific nav link by
its accessible text (e.g., find the "Dashboard" and "Profile" link via
screen.getByRole("link", { name: /Dashboard/i }) / getByRole(... "Profile")) and
assert that the link has aria-current="page" or its className includes "active"
(or both) instead of the tautological checks; keep
mockUseSession/mockUsePathname and authenticatedSession setup but replace the
no-op assertions with explicit active-state expectations so regressions in
DashboardNavbar active highlighting fail the tests.
In `@__tests__/pages/LoginPage.test.tsx`:
- Around line 118-129: The test is selecting generic OAuth "Continue with..."
buttons and asserting the wrong initial state; update the test to target the
email-login submit control specifically and assert its disabled state matches
the component logic in app/login/page.tsx (disabled={loading ||
isEmailAndPasswordEmpty()}). Locate the submit button selection in the test (the
submitBtn variable) and change the query to scope to the email/password form or
match a label/text unique to the email-login submit (instead of the broad /sign
in|log in|login|continue/i), then assert expect(submitBtn).toBeDisabled() when
fields are initially empty (or verify loading false + isEmailAndPasswordEmpty()
behavior) so the test mirrors the isEmailAndPasswordEmpty() disabled logic.
---
Duplicate comments:
In `@__tests__/components/DashboardNavbar.test.tsx`:
- Around line 179-183: The test clicks a generic button via
screen.getByRole("button") which can hit the wrong control (e.g., theme toggle)
and make the sign-out spec flaky; update the test in
__tests__/components/DashboardNavbar.test.tsx to explicitly open the user-menu
trigger before clicking the sign-out item—locate the user menu trigger used by
DashboardNavbar (e.g., the avatar/profile button or the button with an
accessible name like "open user menu" / "account") using a more specific query
such as getByRole("button", { name: /user|account|open user menu|profile/i }) or
a test id used by the component, click that to reveal the menu, then find the
signOutBtn (screen.getByText(/sign out|logout|log out/i)) and click it.
In `@__tests__/components/ProfileCard.test.tsx`:
- Around line 193-200: The test "shows an empty-state message when user has no
links" currently has no assertion; update the test after the render(<ProfileCard
... user={{ ...baseProps.user, links: [] }} />) call to enforce the intended
contract: either assert that the empty-state copy is shown (e.g., use
screen.getByText(/add your first link/i) and expect(...).toBeInTheDocument()) or
assert that no empty copy appears (e.g., use screen.queryByText(/no
links|nothing here|add your first link/i) and expect(...).toBeNull()); pick the
correct contract for this component and add the corresponding expect statement
so the test actually verifies behavior.
In @.github/workflows/jest.yml:
- Line 19: The workflow contains invalid placeholder refs like "uses:
actions/checkout@<full-length-sha>" (and the similar entries at the other
occurrences) which will fail; replace each placeholder with an actual full
40-character commit SHA for the referenced action (e.g.,
actions/checkout@<40-hex-sha>) by looking up the target action repo's commit and
pasting the full SHA into each "uses:" line so every action reference is pinned
to a real commit.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 23c6d3c4-4cc0-47b6-863a-0bca43f5c22b
📒 Files selected for processing (10)
.github/workflows/jest.yml__tests__/components/DashboardNavbar.test.tsx__tests__/components/Navbar.test.tsx__tests__/components/ProfileCard.test.tsx__tests__/components/ThemeToggle.test.tsx__tests__/lib/platforms.test.ts__tests__/lib/url.test.ts__tests__/middleware/csrf.test.ts__tests__/pages/LoginPage.test.tsxjest.setup.ts
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Author
|
@vishnukothakapu & @Anushreebasics |
Anushreebasics
requested changes
May 24, 2026
Anushreebasics
left a comment
Anushreebasics
left a comment
Collaborator
There was a problem hiding this comment.
pls resolve coderabbit reviews and make the CI green
Anushreebasics
requested changes
May 24, 2026
Anushreebasics
left a comment
Anushreebasics
left a comment
Collaborator
There was a problem hiding this comment.
after making the changes, squash commits to only 1
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/jest.yml (1)
19-19:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPin GitHub Actions to full commit SHAs (mutable tags still used).
Line 19, Line 25, and Line 42 still use
@v4tags. Please pin each action to a 40-char commit SHA to satisfy immutability/supply-chain policy.Suggested fix
- uses: actions/checkout@v4 + uses: actions/checkout@<40-char-sha> with: persist-credentials: false @@ - uses: actions/setup-node@v4 + uses: actions/setup-node@<40-char-sha> @@ - uses: actions/upload-artifact@v4 + uses: actions/upload-artifact@<40-char-sha>#!/bin/bash set -euo pipefail file=".github/workflows/jest.yml" echo "Checking for mutable action tags (should be no output):" rg -nP '^\s*uses:\s*actions/(checkout|setup-node|upload-artifact)`@v`[0-9]+' "$file" || true echo echo "Checking for SHA-pinned action refs (should show 3 lines):" rg -nP '^\s*uses:\s*actions/(checkout|setup-node|upload-artifact)@[0-9a-f]{40}$' "$file" || trueAlso applies to: 25-25, 42-42
🤖 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 @.github/workflows/jest.yml at line 19, The workflow uses mutable tags for actions (uses: actions/checkout@v4, actions/setup-node@v4, actions/upload-artifact@v4); replace each `@v4` reference with the corresponding full 40-character commit SHA for that action so the workflow is pinned to an immutable rev (update the three occurrences currently at lines showing uses: actions/(checkout|setup-node|upload-artifact)`@v4`), ensure the SHA strings are exact 40 hex chars and keep YAML syntax intact.
🤖 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.
Duplicate comments:
In @.github/workflows/jest.yml:
- Line 19: The workflow uses mutable tags for actions (uses:
actions/checkout@v4, actions/setup-node@v4, actions/upload-artifact@v4); replace
each `@v4` reference with the corresponding full 40-character commit SHA for that
action so the workflow is pinned to an immutable rev (update the three
occurrences currently at lines showing uses:
actions/(checkout|setup-node|upload-artifact)`@v4`), ensure the SHA strings are
exact 40 hex chars and keep YAML syntax intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 688fb741-cd24-489e-9d45-95538fb7f5ec
📒 Files selected for processing (1)
.github/workflows/jest.yml
Author
|
@vishnukothakapu & @Anushreebasics |
Collaborator
Collaborator
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Sets up a minimal but scalable Jest + React Testing Library infrastructure
for the Next.js App Router codebase. Adds configuration, global mocks,
and foundational tests to validate the setup and demonstrate patterns
for future contributors.
Related Issue
Closes #206
Type of Change
Files Added
jest.config.tsnext/jest— handles SWC transform,@/alias, envjest.setup.tsnext/navigation,next-auth/react,next/image__tests__/lib/platforms.test.ts__tests__/lib/url.test.ts__tests__/components/Navbar.test.tsx__tests__/middleware/csrf.test.ts.github/workflows/jest.ymlTESTING.mdFiles Modified
package.jsontest:jest,test:jest:watch,test:jest:coverage,test:allscripts + 5 devDependenciesHow to Test
npm installnpm run test:jest— all tests should passnpm run test:jest:coverage— check the coverage report in./coverage/npm run test:all— runs both tsx tests and Jest tests togetherDesign Decisions
tsx --testrunner —npm testis untouched; Jest runs separately vianpm run test:jestnext/jestwrapper — mandatory for Next.js App Router; handles module transforms and env loading automatically@testing-library/react@^16which officially supports React 19jest.config.ts; keeps the bar achievable now but raises it over timelib/auth.tsare excluded from test coverage to avoid env requirements in CIScreenshots
N/A (no UI change)
Checklist
Summary by CodeRabbit
Documentation
Tests
Chores