Skip to content

🧹 [code health improvement] Refactor LayoutEditor tests to use proper typing instead of any #293

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
is0692vs wants to merge 2 commits into from
+19 −27
Closed

🧹 [code health improvement] Refactor LayoutEditor tests to use proper typing instead of any #293

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d852470
🧹 fix: remove any types and eslint-disable in LayoutEditor.test.tsx
google-labs-jules[bot] May 22, 2026
e0963f0
🧹 fix: remove any types and eslint-disable in LayoutEditor.test.tsx
google-labs-jules[bot] May 22, 2026
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
46 changes: 19 additions & 27 deletions src/components/__tests__/LayoutEditor.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,7 @@ vi.mock("@dnd-kit/core", async (importOriginal) => {
useSensor: vi.fn(() => ({})),
useDroppable: ({ id }: { id: string }) => ({
setNodeRef: vi.fn(),
// eslint-disable-next-line @typescript-eslint/no-explicit-any
isOver: (window as any).mockIsOverId === id,
isOver: (window as unknown as { mockIsOverId?: string }).mockIsOverId === id,
}),
PointerSensor: vi.fn(),
KeyboardSensor: vi.fn(),
Expand Down Expand Up @@ -58,16 +57,16 @@ const defaultLayout: CardLayout = {
};

describe("LayoutEditor", () => {
type MockWindow = typeof window & { triggerDragEnd?: (event: unknown) => void; mockIsOverId?: string; };
const mockWindow = window as unknown as MockWindow;
Comment on lines +60 to +61
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot May 22, 2026

Choose a reason for hiding this comment

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

P2 MockWindow 型のスコープが限定的

MockWindow 型が describe ブロック内で定義されているため、ファイル先頭の vi.mock ファクトリ（行16）では利用できません。そのため、ファクトリ内では別のインライン型キャスト (window as unknown as { triggerDragEnd: (event: unknown) => void }) が引き続き使用されています。MockWindow をモジュールトップレベルに移動すれば、ファクトリ内でも同じ型を再利用でき、完全な一貫性が確保されます。

Prompt To Fix With AI 
This is a comment left during a code review.
Path: src/components/__tests__/LayoutEditor.test.tsx
Line: 60-61

Comment:
**`MockWindow` 型のスコープが限定的**

`MockWindow` 型が `describe` ブロック内で定義されているため、ファイル先頭の `vi.mock` ファクトリ（行16）では利用できません。そのため、ファクトリ内では別のインライン型キャスト `(window as unknown as { triggerDragEnd: (event: unknown) => void })` が引き続き使用されています。`MockWindow` をモジュールトップレベルに移動すれば、ファクトリ内でも同じ型を再利用でき、完全な一貫性が確保されます。

How can I resolve this? If you propose a fix, please make it concise.

let mockOnLayoutChange: ReturnType<typeof vi.fn>;
let mockOnToggleVisibility: ReturnType<typeof vi.fn>;

beforeEach(() => {
mockOnLayoutChange = vi.fn();
mockOnToggleVisibility = vi.fn();
// eslint-disable-next-line @typescript-eslint/no-explicit-any
(window as any).triggerDragEnd = undefined;
// eslint-disable-next-line @typescript-eslint/no-explicit-any
(window as any).mockIsOverId = undefined;
mockWindow.triggerDragEnd = undefined;
mockWindow.mockIsOverId = undefined;
});

it("renders blocks in their respective columns", () => {
Expand Down Expand Up @@ -119,12 +118,11 @@ describe("LayoutEditor", () => {
const dndContext = screen.getByTestId("dnd-context");
fireEvent.click(dndContext);

// eslint-disable-next-line @typescript-eslint/no-explicit-any
const triggerDragEnd = (window as any).triggerDragEnd;
const triggerDragEnd = mockWindow.triggerDragEnd;
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot May 22, 2026

Choose a reason for hiding this comment

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

medium

Since triggerDragEnd is defined as optional in the MockWindow type, calling it directly on line 125 will trigger a TypeScript error (e.g., 'Cannot invoke an object which is possibly undefined'). Since you've asserted its presence on the next line, you should use a non-null assertion here to satisfy the compiler.

Suggested change
const triggerDragEnd = mockWindow.triggerDragEnd;
const triggerDragEnd = mockWindow.triggerDragEnd!;

expect(triggerDragEnd).toBeDefined();

// Drag 'avatar' to 'right' column
triggerDragEnd({
triggerDragEnd!({
active: { id: "avatar" },
over: { id: "right" },
});
Expand All @@ -149,11 +147,10 @@ describe("LayoutEditor", () => {
const dndContext = screen.getByTestId("dnd-context");
fireEvent.click(dndContext);

// eslint-disable-next-line @typescript-eslint/no-explicit-any
const triggerDragEnd = (window as any).triggerDragEnd;
const triggerDragEnd = mockWindow.triggerDragEnd;
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot May 22, 2026

Choose a reason for hiding this comment

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

medium

As noted in previous tests, triggerDragEnd is optional in the type definition. Use a non-null assertion to avoid TypeScript errors when calling it.

Suggested change
const triggerDragEnd = mockWindow.triggerDragEnd;
const triggerDragEnd = mockWindow.triggerDragEnd!;


// Drag 'avatar' over 'topLanguages'
triggerDragEnd({
triggerDragEnd!({
active: { id: "avatar" },
over: { id: "topLanguages" },
});
Expand All @@ -178,9 +175,8 @@ describe("LayoutEditor", () => {
const dndContext = screen.getByTestId("dnd-context");
fireEvent.click(dndContext);

// eslint-disable-next-line @typescript-eslint/no-explicit-any
const triggerDragEnd = (window as any).triggerDragEnd;
triggerDragEnd({
const triggerDragEnd = mockWindow.triggerDragEnd;
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot May 22, 2026

Choose a reason for hiding this comment

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

medium

As noted in previous tests, triggerDragEnd is optional in the type definition. Use a non-null assertion to avoid TypeScript errors when calling it.

Suggested change
const triggerDragEnd = mockWindow.triggerDragEnd;
const triggerDragEnd = mockWindow.triggerDragEnd!;

triggerDragEnd!({
active: { id: "avatar" },
over: null,
});
Expand All @@ -200,9 +196,8 @@ describe("LayoutEditor", () => {
const dndContext = screen.getByTestId("dnd-context");
fireEvent.click(dndContext);

// eslint-disable-next-line @typescript-eslint/no-explicit-any
const triggerDragEnd = (window as any).triggerDragEnd;
triggerDragEnd({
const triggerDragEnd = mockWindow.triggerDragEnd;
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot May 22, 2026

Choose a reason for hiding this comment

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

medium

As noted in previous tests, triggerDragEnd is optional in the type definition. Use a non-null assertion to avoid TypeScript errors when calling it.

Suggested change
const triggerDragEnd = mockWindow.triggerDragEnd;
const triggerDragEnd = mockWindow.triggerDragEnd!;

triggerDragEnd!({
active: { id: "avatar" },
over: { id: "avatar" },
});
Expand All @@ -222,9 +217,8 @@ describe("LayoutEditor", () => {
const dndContext = screen.getByTestId("dnd-context");
fireEvent.click(dndContext);

// eslint-disable-next-line @typescript-eslint/no-explicit-any
const triggerDragEnd = (window as any).triggerDragEnd;
triggerDragEnd({
const triggerDragEnd = mockWindow.triggerDragEnd;
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot May 22, 2026

Choose a reason for hiding this comment

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

medium

As noted in previous tests, triggerDragEnd is optional in the type definition. Use a non-null assertion to avoid TypeScript errors when calling it.

Suggested change
const triggerDragEnd = mockWindow.triggerDragEnd;
const triggerDragEnd = mockWindow.triggerDragEnd!;

triggerDragEnd!({
active: { id: "avatar" },
over: { id: "non-existent-block" },
});
Expand All @@ -250,11 +244,10 @@ describe("LayoutEditor", () => {
const dndContext = screen.getByTestId("dnd-context");
fireEvent.click(dndContext);

// eslint-disable-next-line @typescript-eslint/no-explicit-any
const triggerDragEnd = (window as any).triggerDragEnd;
const triggerDragEnd = mockWindow.triggerDragEnd;
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot May 22, 2026

Choose a reason for hiding this comment

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

medium

As noted in previous tests, triggerDragEnd is optional in the type definition. Use a non-null assertion to avoid TypeScript errors when calling it.

Suggested change
const triggerDragEnd = mockWindow.triggerDragEnd;
const triggerDragEnd = mockWindow.triggerDragEnd!;


// Drag 'avatar' over 'stats' (downwards)
triggerDragEnd({
triggerDragEnd!({
active: { id: "avatar" },
over: { id: "stats" },
});
Expand All @@ -278,11 +271,10 @@ describe("LayoutEditor", () => {
const dndContext = screen.getByTestId("dnd-context");
fireEvent.click(dndContext);

// eslint-disable-next-line @typescript-eslint/no-explicit-any
const triggerDragEnd = (window as any).triggerDragEnd;
const triggerDragEnd = mockWindow.triggerDragEnd;
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot May 22, 2026

Choose a reason for hiding this comment

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

medium

As noted in previous tests, triggerDragEnd is optional in the type definition. Use a non-null assertion to avoid TypeScript errors when calling it.

Suggested change
const triggerDragEnd = mockWindow.triggerDragEnd;
const triggerDragEnd = mockWindow.triggerDragEnd!;


// Drag 'avatar' to empty column 'right'
triggerDragEnd({
triggerDragEnd!({
active: { id: "avatar" },
over: { id: "right" },
});
Expand Down
Loading