Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions src/components/atoms/Label/Label.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
import {render, screen} from "@testing-library/react";
import {describe, expect, it} from "vitest";
import {Label} from "./Label";

describe("Label Atom", () => {
it("renders the label text correctly", () => {
render(<Label htmlFor="email">Email Address</Label>);

expect(screen.getByText("Email Address")).toBeInTheDocument();
});
Comment on lines +6 to +10
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Use "should" prefix for test names and semantic queries.

Test names should start with "should" per coding guidelines. Additionally, prefer semantic queries like getByLabelText or getByRole over getByText for better accessibility testing.

As per coding guidelines, test names should be descriptive and start with 'should'.

♻️ Proposed refactor with semantic queries and proper naming
-    it("renders the label text correctly", () => {
+    it("should render the label text correctly", () => {
         render(<Label htmlFor="email">Email Address</Label>);
 
-        expect(screen.getByText("Email Address")).toBeInTheDocument();
+        const label = screen.getByLabelText("Email Address", { selector: 'label' });
+        expect(label).toBeInTheDocument();
     });
 
-    it("applies the htmlFor attribute correctly", () => {
+    it("should apply the htmlFor attribute correctly", () => {
         render(<Label htmlFor="email">Email</Label>);
 
         const label = screen.getByText("Email");
         expect(label).toHaveAttribute("for", "email");
     });
 
-    it("shows required indicator when required prop is true", () => {
+    it("should show required indicator when required prop is true", () => {
         render(
             <Label htmlFor="email" required>
                 Email
             </Label>
         );
 
         expect(screen.getByText("*")).toBeInTheDocument();
     });
 
-    it("does not show required indicator when required is false", () => {
+    it("should not show required indicator when required is false", () => {
         render(<Label htmlFor="email">Email</Label>);
 
         expect(screen.queryByText("*")).not.toBeInTheDocument();
     });

Also applies to: 12-17, 19-27, 29-33

🤖 Prompt for AI Agents
In @src/components/atoms/Label/Label.test.tsx around lines 6 - 10, The test in
Label.test.tsx uses a non-semantic name and query: rename the spec for the Label
component to start with "should" (e.g., "should render the label text for the
associated control") and replace screen.getByText("Email Address") with an
accessibility-focused query such as screen.getByLabelText or screen.getByRole
(targeting the associated input/role by accessible name) to assert the label is
tied to the control; apply the same naming and semantic-query changes to the
other tests referenced (lines 12-17, 19-27, 29-33) so all tests use "should"
prefixes and prefer getByLabelText/getByRole over getByText.

Comment on lines +6 to +10
Copy link

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test descriptions should start with "should" for consistency with the project's TDD patterns. For example: "should render the label text correctly" instead of "renders the label text correctly". This follows the AAA (Arrange-Act-Assert) pattern used in other test files like Button.test.tsx.

Copilot uses AI. Check for mistakes.

it("applies the htmlFor attribute correctly", () => {
render(<Label htmlFor="email">Email</Label>);

const label = screen.getByText("Email");
expect(label).toHaveAttribute("for", "email");
});

it("shows required indicator when required prop is true", () => {
render(
<Label htmlFor="email" required>
Email
</Label>
);

expect(screen.getByText("*")).toBeInTheDocument();
});

it("does not show required indicator when required is false", () => {
render(<Label htmlFor="email">Email</Label>);

expect(screen.queryByText("*")).not.toBeInTheDocument();
});
});
Comment on lines +5 to +34
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Add missing test coverage for variants, disabled state, default props, and accessibility.

Per coding guidelines, component tests must include coverage for default props, variants, disabled state, and accessibility. The Label component has a size prop with variants ("sm", "md", "lg"), a disabled prop, and a default size of "md", but these are not tested.

📋 Suggested additional tests

Add the following test cases to ensure complete coverage:

it("should apply default size (md) when no size prop is provided", () => {
    render(<Label htmlFor="email">Email</Label>);
    const label = screen.getByRole('label', { name: /email/i });
    expect(label).toHaveClass('text-sm'); // md size maps to text-sm
});

it("should apply small size styling", () => {
    render(<Label htmlFor="email" size="sm">Email</Label>);
    const label = screen.getByRole('label', { name: /email/i });
    expect(label).toHaveClass('text-xs');
});

it("should apply large size styling", () => {
    render(<Label htmlFor="email" size="lg">Email</Label>);
    const label = screen.getByRole('label', { name: /email/i });
    expect(label).toHaveClass('text-base');
});

it("should apply disabled styling when disabled prop is true", () => {
    render(<Label htmlFor="email" disabled>Email</Label>);
    const label = screen.getByRole('label', { name: /email/i });
    expect(label).toHaveClass('opacity-50');
    expect(label).toHaveClass('cursor-not-allowed');
});

it("should apply custom className", () => {
    render(<Label htmlFor="email" className="custom-class">Email</Label>);
    const label = screen.getByRole('label', { name: /email/i });
    expect(label).toHaveClass('custom-class');
});

Based on coding guidelines.

🤖 Prompt for AI Agents
In @src/components/atoms/Label/Label.test.tsx around lines 5 - 34, Tests for the
Label atom are missing coverage for default props, size variants, disabled
state, custom className and accessibility; update
src/components/atoms/Label/Label.test.tsx to add tests that render the Label
component and assert its role and classes: use render(<Label ... />) and
screen.getByRole('label', { name: /email/i }) to verify the default size maps to
the "md" styling (expect class like "text-sm"), verify size="sm" yields
"text-xs" and size="lg" yields "text-base", verify disabled prop adds
"opacity-50" and "cursor-not-allowed", and verify passing className (e.g.
"custom-class") is applied; keep existing tests for text, htmlFor and required
indicator and ensure assertions use toHaveClass / toHaveAttribute /
toBeInTheDocument as appropriate.

58 changes: 58 additions & 0 deletions src/components/atoms/Label/Label.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
// src/components/atoms/Label/Label.tsx
import type {LabelProps} from "./LabelProps";

const sizeClasses: Record<NonNullable<LabelProps["size"]>, string> = {
sm: "text-xs",
md: "text-sm",
lg: "text-base",
};

/**
* A form label component that supports various sizes, required indicators, and disabled states.
* @see LabelProps
* @example
* <Label htmlFor="email" size="lg">
* Email Address
* </Label>
*/
export default function Label({
htmlFor,
children,
required,
size = "md",
disabled,
className,
}: Readonly<LabelProps>) {
return (
<label
htmlFor={htmlFor}
className={[
"block font-medium text-slate-700 dark:text-slate-300",
sizeClasses[size],
disabled && "opacity-50 cursor-not-allowed",
className,
]
.filter(Boolean)
.join(" ")}
>
{children}

{required && (
<>
{/* Screen readers only */}
<span className="sr-only">(required)</span>

{/* Visual required indicator */}
<span
aria-hidden="true"
className="ml-1 text-red-500"
>
*
</span>
</>


)}
</label>
);
}
24 changes: 24 additions & 0 deletions src/components/atoms/Label/LabelProps.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// src/components/atoms/Label/LabelProps.ts
import type {ReactNode} from "react";

export type LabelSize = "sm" | "md" | "lg";

export interface LabelProps {
/** id of the input this label is associated with */
htmlFor?: string;

/** label text */
children: ReactNode;

/** show required asterisk */
required?: boolean;

/** visual size */
size?: LabelSize;

/** disabled appearance */
disabled?: boolean;

/** extra tailwind classes */
className?: string;
}
56 changes: 56 additions & 0 deletions src/components/layout/Header/Header.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
import { render, screen, fireEvent } from "@testing-library/react";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Import userEvent instead of using fireEvent.

Per coding guidelines, use @testing-library/user-event for user interactions, not fireEvent. This provides more realistic user interaction simulation.

As per coding guidelines, use @testing-library/user-event for user interactions in tests.

♻️ Update imports
-import { render, screen, fireEvent } from "@testing-library/react";
+import { render, screen } from "@testing-library/react";
+import userEvent from "@testing-library/user-event";
 import { describe, it, expect, vi } from "vitest";
 import Header from "./Header";

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In @src/components/layout/Header/Header.test.tsx at line 1, Replace the use of
fireEvent with @testing-library/user-event: update the import line to import
userEvent (instead of fireEvent) alongside render and screen, then replace
occurrences of fireEvent.* in Header.test.tsx with the corresponding userEvent.*
calls (e.g., userEvent.click) and make the test async/await where necessary to
account for userEvent's async behavior so interactions behave like real users.

Copy link

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test file is missing the import for userEvent from '@testing-library/user-event', which is needed for proper user interaction testing according to the project's coding guidelines. Add the import:

import userEvent from '@testing-library/user-event';

This is required for following the guideline: "Use @testing-library/user-event for user interactions".

Copilot uses AI. Check for mistakes.
import { describe, it, expect, vi } from "vitest";
import Header from "./Header";
Copy link

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The import statement uses a default import instead of a named import. Based on the project's coding guidelines, components should be exported as named exports using export function ComponentName(). The import should be:

import { Header } from "./Header";

This follows the pattern established in other components like Button.

Suggested change
import Header from "./Header";
import { Header } from "./Header";

Copilot uses AI. Check for mistakes.

describe("Header component", () => {
const mockLogout = vi.fn();

const defaultProps = {
user: { name: "Karuna" },
onLogout: mockLogout,
};

it("renders the navbar container", () => {
render(<Header {...defaultProps} />);
expect(screen.getByRole("banner")).toBeInTheDocument();
});
Comment on lines +13 to +16
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Use "should" prefix for test names.

Per coding guidelines, test names should be descriptive and start with "should".

As per coding guidelines, use descriptive test names starting with 'should'.

♻️ Update test names
-    it("renders the navbar container", () => {
+    it("should render the navbar container", () => {
         render(<Header {...defaultProps} />);
         expect(screen.getByRole("banner")).toBeInTheDocument();
     });
 
-    it("renders the app title", () => {
+    it("should render the app title", () => {
         render(<Header {...defaultProps} />);
         expect(screen.getByText("daisyUI")).toBeInTheDocument();
     });
 
-    it("renders the hamburger menu link", () => {
+    it("should render the hamburger menu link", () => {
         render(<Header {...defaultProps} />);
         const menuLink = screen.getAllByRole("link")[0];
         expect(menuLink).toHaveAttribute("href", "/");
     });
 
-    it("renders profile button", () => {
+    it("should render profile button", () => {
         render(<Header {...defaultProps} />);
         expect(screen.getByRole("button")).toBeInTheDocument();
     });
 
-    it("shows the first letter of the user's name in profile image", () => {
+    it("should show the first letter of the user's name in profile image", () => {
         render(<Header {...defaultProps} />);
         const profileImg = screen.getByAltText("Karuna profile");
         expect(profileImg).toHaveAttribute(
             "src",
             expect.stringContaining("K")
         );
     });
 
-    it("uses fallback text when user is undefined", () => {
+    it("should use fallback text when user is undefined", () => {
         render(<Header onLogout={mockLogout} />);
         expect(screen.getByAltText("User profile")).toBeInTheDocument();
     });
 
-    it("calls onLogout when profile button is clicked", () => {
+    it("should call onLogout when profile button is clicked", async () => {
         render(<Header {...defaultProps} />);
         const profileButton = screen.getByRole("button");
 
-        fireEvent.click(profileButton);
+        await userEvent.click(profileButton);
 
         expect(mockLogout).toHaveBeenCalledOnce();
     });

Also applies to: 18-21, 23-27, 29-32, 34-41, 43-46, 48-55

🤖 Prompt for AI Agents
In @src/components/layout/Header/Header.test.tsx around lines 13 - 16, Rename
the test titles to start with "should" and make them descriptive: e.g., change
it("renders the navbar container", ...) to it("should render the navbar
container", ...) and similarly update the other test descriptions around the
Header component tests that call render(<Header {...defaultProps} />) and
assertions like expect(screen.getByRole("banner")).toBeInTheDocument(); and the
tests that check links, buttons, and conditional items; keep the test bodies
unchanged, only update the string descriptions for lines mentioned (and the
other ranges) so all test names begin with "should".

Comment on lines +13 to +16
Copy link

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the project's testing standards, test descriptions should start with "should" for consistency. For example: "should render the navbar container" instead of "renders the navbar container". This pattern is established in the coding guidelines and helps maintain a consistent AAA (Arrange-Act-Assert) testing pattern throughout the project.

Copilot uses AI. Check for mistakes.

it("renders the app title", () => {
render(<Header {...defaultProps} />);
expect(screen.getByText("daisyUI")).toBeInTheDocument();
});

it("renders the hamburger menu link", () => {
render(<Header {...defaultProps} />);
const menuLink = screen.getAllByRole("link")[0];
Copy link

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using getAllByRole with array indexing [0] is fragile and can lead to brittle tests. Instead, use a more specific query that uniquely identifies the hamburger menu link, such as checking for the specific link that contains the SVG icon or using a more descriptive role query. Consider using getByRole('link', { name: /menu/i }) if the link has accessible text, or add a test-id if necessary as a last resort.

Suggested change
const menuLink = screen.getAllByRole("link")[0];
const menuLink = screen.getByRole("link", { name: /daisyUI/i });

Copilot uses AI. Check for mistakes.
expect(menuLink).toHaveAttribute("href", "/");
});

it("renders profile button", () => {
render(<Header {...defaultProps} />);
expect(screen.getByRole("button")).toBeInTheDocument();
});

it("shows the first letter of the user's name in profile image", () => {
render(<Header {...defaultProps} />);
const profileImg = screen.getByAltText("Karuna profile");
expect(profileImg).toHaveAttribute(
"src",
expect.stringContaining("K")
);
});

it("uses fallback text when user is undefined", () => {
render(<Header onLogout={mockLogout} />);
expect(screen.getByAltText("User profile")).toBeInTheDocument();
});

it("calls onLogout when profile button is clicked", () => {
render(<Header {...defaultProps} />);
const profileButton = screen.getByRole("button");

fireEvent.click(profileButton);
Copy link

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the project's coding guidelines, fireEvent should be replaced with userEvent from '@testing-library/user-event' for more realistic user interaction simulation. The guidelines specifically state "Use @testing-library/user-event for user interactions" and "Always use async/await with user events".

The code should be:

const user = userEvent.setup();
await user.click(profileButton);

Note that the test function would also need to be made async.

Copilot uses AI. Check for mistakes.

expect(mockLogout).toHaveBeenCalledOnce();
});
Comment on lines +48 to +55
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Use userEvent.click() with async/await instead of fireEvent.click().

Per coding guidelines, always use async/await with user events. Replace fireEvent.click() with userEvent.click() for more realistic user interaction simulation.

As per coding guidelines, always use async/await with user events in tests.

♻️ Refactor to use userEvent
-    it("calls onLogout when profile button is clicked", () => {
+    it("calls onLogout when profile button is clicked", async () => {
         render(<Header {...defaultProps} />);
         const profileButton = screen.getByRole("button");
 
-        fireEvent.click(profileButton);
+        await userEvent.click(profileButton);
 
         expect(mockLogout).toHaveBeenCalledOnce();
     });
📝 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.

Suggested change
it("calls onLogout when profile button is clicked", () => {
render(<Header {...defaultProps} />);
const profileButton = screen.getByRole("button");
fireEvent.click(profileButton);
expect(mockLogout).toHaveBeenCalledOnce();
});
it("calls onLogout when profile button is clicked", async () => {
render(<Header {...defaultProps} />);
const profileButton = screen.getByRole("button");
await userEvent.click(profileButton);
expect(mockLogout).toHaveBeenCalledOnce();
});
🤖 Prompt for AI Agents
In @src/components/layout/Header/Header.test.tsx around lines 48 - 55, The test
should use async/await with userEvent instead of fireEvent: make the test
callback async, replace fireEvent.click(profileButton) with await
userEvent.click(profileButton), and ensure userEvent is imported (or available)
in Header.test.tsx so the user interaction is simulated realistically when
asserting mockLogout in the "calls onLogout when profile button is clicked"
test.

});