🧪 test: add tests for useCardSettings hook#341
Conversation
Co-authored-by: is0692vs <135803462+is0692vs@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Warning Review limit reached
More reviews will be available in 51 minutes. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive test suite for the useCardSettings hook in src/hooks/__tests__/useCardSettings.test.ts. The reviewer feedback focuses on improving TypeScript type safety and code cleanliness by importing proper types (CardLayout, CardDisplayOptions, and CardBlockId), explicitly typing mock data and mock factory functions, and removing unnecessary @ts-expect-error annotations throughout the test file.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| import { useCardSettings } from "../useCardSettings"; | ||
| import * as cardSettingsLib from "@/lib/cardSettings"; | ||
| import * as cardLayoutLib from "@/lib/cardLayout"; |
There was a problem hiding this comment.
Import the required types CardLayout, CardDisplayOptions, and CardBlockId from @/lib/types to enable proper typing of mock data and eliminate the need for @ts-expect-error annotations.
| import { useCardSettings } from "../useCardSettings"; | |
| import * as cardSettingsLib from "@/lib/cardSettings"; | |
| import * as cardLayoutLib from "@/lib/cardLayout"; | |
| import { useCardSettings } from "../useCardSettings"; | |
| import * as cardSettingsLib from "@/lib/cardSettings"; | |
| import * as cardLayoutLib from "@/lib/cardLayout"; | |
| import type { CardLayout, CardDisplayOptions, CardBlockId } from "@/lib/types"; |
References
- Maintain explicit return types for functions in TypeScript to ensure type safety and API clarity. If a type is only used for annotations, use 'import type' instead of removing the annotation to satisfy linting rules.
| vi.mock("@/lib/cardSettings", () => ({ | ||
| loadCardSettings: vi.fn(), | ||
| saveCardSettings: vi.fn(), | ||
| })); | ||
|
|
||
| vi.mock("@/lib/cardLayout", () => ({ | ||
| toggleBlockVisibility: vi.fn(), | ||
| })); |
There was a problem hiding this comment.
According to the general rules, mock implementations and functions in TypeScript should have explicit return types to maintain type safety and readability. Please add explicit return types to the mock factory functions.
| vi.mock("@/lib/cardSettings", () => ({ | |
| loadCardSettings: vi.fn(), | |
| saveCardSettings: vi.fn(), | |
| })); | |
| vi.mock("@/lib/cardLayout", () => ({ | |
| toggleBlockVisibility: vi.fn(), | |
| })); | |
| vi.mock("@/lib/cardSettings", (): Record<string, any> => ({ | |
| loadCardSettings: vi.fn(), | |
| saveCardSettings: vi.fn(), | |
| })); | |
| vi.mock("@/lib/cardLayout", (): Record<string, any> => ({ | |
| toggleBlockVisibility: vi.fn(), | |
| })); |
References
- In TypeScript, ensure functions and mock implementations have explicit return types and use async functions for mocks returning Promises to maintain type safety and readability.
| const mockDefaultLayout = { | ||
| blocks: [ | ||
| { id: "profile", visible: true, column: "full" }, | ||
| { id: "stats", visible: false, column: "left" }, | ||
| ], | ||
| }; | ||
|
|
||
| const mockDefaultOptions = { | ||
| showAvatar: true, | ||
| showBio: false, | ||
| }; |
There was a problem hiding this comment.
Type mockDefaultLayout and mockDefaultOptions properly using CardLayout and CardDisplayOptions (with cardSettingsLib.DEFAULT_DISPLAY_OPTIONS as a base). This will allow us to safely remove all @ts-expect-error comments in this file.
| const mockDefaultLayout = { | |
| blocks: [ | |
| { id: "profile", visible: true, column: "full" }, | |
| { id: "stats", visible: false, column: "left" }, | |
| ], | |
| }; | |
| const mockDefaultOptions = { | |
| showAvatar: true, | |
| showBio: false, | |
| }; | |
| const mockDefaultLayout: CardLayout = { | |
| blocks: [ | |
| { id: "profile", visible: true, column: "full" }, | |
| { id: "stats", visible: false, column: "left" }, | |
| ], | |
| }; | |
| const mockDefaultOptions: CardDisplayOptions = { | |
| ...cardSettingsLib.DEFAULT_DISPLAY_OPTIONS, | |
| showAvatar: true, | |
| showBio: false, | |
| }; |
| vi.mocked(cardSettingsLib.loadCardSettings).mockReturnValue({ | ||
| // @ts-expect-error - mock data | ||
| layout: mockDefaultLayout, | ||
| // @ts-expect-error - mock data | ||
| options: mockDefaultOptions, | ||
| }); |
There was a problem hiding this comment.
With properly typed mock data, we can remove the @ts-expect-error annotations here.
vi.mocked(cardSettingsLib.loadCardSettings).mockReturnValue({
layout: mockDefaultLayout,
options: mockDefaultOptions,
});| act(() => { | ||
| // @ts-expect-error - mock data | ||
| result.current.setDisplayOptions({ showAvatar: false, showBio: false }); | ||
| }); |
There was a problem hiding this comment.
Pass a complete CardDisplayOptions object using the spread operator to avoid the @ts-expect-error annotation.
act(() => {
result.current.setDisplayOptions({
...mockDefaultOptions,
showAvatar: false,
showBio: false,
});
});| act(() => { | ||
| // @ts-expect-error - mock data | ||
| result.current.toggleMainBlockVisibility("profile"); | ||
| }); |
There was a problem hiding this comment.
Remove the unnecessary @ts-expect-error annotation as "profile" is a valid CardBlockId.
act(() => {
result.current.toggleMainBlockVisibility("profile");
});| act(() => { | ||
| // @ts-expect-error - mock data | ||
| result.current.toggleDisplayOption("showAvatar"); | ||
| }); | ||
|
|
||
| expect(result.current.displayOptions.showAvatar).toBe(false); | ||
|
|
||
| act(() => { | ||
| // @ts-expect-error - mock data | ||
| result.current.toggleDisplayOption("showBio"); | ||
| }); |
There was a problem hiding this comment.
Remove the unnecessary @ts-expect-error annotations as "showAvatar" and "showBio" are valid keys of CardDisplayOptions.
act(() => {
result.current.toggleDisplayOption("showAvatar");
});
expect(result.current.displayOptions.showAvatar).toBe(false);
act(() => {
result.current.toggleDisplayOption("showBio");
});| // @ts-expect-error - mock data | ||
| expect(result.current.isBlockVisible("profile")).toBe(true); | ||
| // @ts-expect-error - mock data | ||
| expect(result.current.isBlockVisible("stats")).toBe(false); | ||
| // @ts-expect-error - mock data | ||
| expect(result.current.isBlockVisible("unknown")).toBe(false); | ||
| }); |
There was a problem hiding this comment.
Remove the unnecessary @ts-expect-error annotations for valid block IDs, and use a type assertion (as CardBlockId) for the invalid "unknown" block ID to keep the test type-safe.
| // @ts-expect-error - mock data | |
| expect(result.current.isBlockVisible("profile")).toBe(true); | |
| // @ts-expect-error - mock data | |
| expect(result.current.isBlockVisible("stats")).toBe(false); | |
| // @ts-expect-error - mock data | |
| expect(result.current.isBlockVisible("unknown")).toBe(false); | |
| }); | |
| expect(result.current.isBlockVisible("profile")).toBe(true); | |
| expect(result.current.isBlockVisible("stats")).toBe(false); | |
| expect(result.current.isBlockVisible("unknown" as CardBlockId)).toBe(false); |
Co-authored-by: is0692vs <135803462+is0692vs@users.noreply.github.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const mockDefaultLayout: import("@/lib/types").CardLayout = { | ||
| blocks: [ | ||
| { id: "profile" as import("@/lib/types").CardBlockId, visible: true, column: "full" }, | ||
| { id: "stats" as import("@/lib/types").CardBlockId, visible: false, column: "left" }, | ||
| ], | ||
| }; | ||
|
|
||
| const mockDefaultOptions = { | ||
| showAvatar: true, | ||
| showBio: false, | ||
| }; | ||
|
|
||
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
| vi.mocked(cardSettingsLib.loadCardSettings).mockReturnValue({ | ||
| layout: mockDefaultLayout, | ||
| options: mockDefaultOptions, | ||
| }); | ||
| }); | ||
|
|
||
| it("should initialize with values from loadCardSettings synchronously", () => { | ||
| const { result } = renderHook(() => useCardSettings(false)); | ||
|
|
||
| // loadCardSettings is called twice during useState lazy initialization (once for layout, once for options) | ||
| expect(cardSettingsLib.loadCardSettings).toHaveBeenCalledTimes(2); | ||
| expect(result.current.layout).toEqual(mockDefaultLayout); | ||
| expect(result.current.displayOptions).toEqual(mockDefaultOptions); | ||
| expect(cardSettingsLib.saveCardSettings).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("should not hydrate if mounted is false", () => { | ||
| const { result } = renderHook(() => useCardSettings(false)); | ||
|
|
||
| expect(cardSettingsLib.loadCardSettings).toHaveBeenCalledTimes(2); // Only for init | ||
| // Hydration useEffect should return early | ||
| expect(result.current.layout).toEqual(mockDefaultLayout); | ||
| expect(result.current.displayOptions).toEqual(mockDefaultOptions); | ||
| expect(cardSettingsLib.saveCardSettings).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
There was a problem hiding this comment.
"should initialize with values from loadCardSettings synchronously"（1番目）と "should not hydrate if mounted is false"（2番目）は、全く同じセットアップ（mounted: false）で全く同じアサーション（loadCardSettings 2回呼び出し・layout・displayOptions・saveCardSettings 未呼び出し）を行っています。2つのテストが独立した観点を持つように見えますが、実際には内容が重複しており、どちらか一方を削除するかアサーションを統合すべきです。
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/hooks/__tests__/useCardSettings.test.ts
Line: 18-57
Comment:
**重複している最初の2つのテスト**
"should initialize with values from loadCardSettings synchronously"（1番目）と "should not hydrate if mounted is false"（2番目）は、全く同じセットアップ（`mounted: false`）で全く同じアサーション（`loadCardSettings` 2回呼び出し・`layout`・`displayOptions`・`saveCardSettings` 未呼び出し）を行っています。2つのテストが独立した観点を持つように見えますが、実際には内容が重複しており、どちらか一方を削除するかアサーションを統合すべきです。
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| expect(result.current.layout).toEqual(mockDefaultLayout); | ||
| expect(result.current.displayOptions).toEqual(mockDefaultOptions); | ||
| expect(cardSettingsLib.saveCardSettings).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
loadCardSettings が useState の遅延初期化で2回呼ばれることを toHaveBeenCalledTimes(2) でハードコードしています。これはフックの内部実装の詳細（2つの useState がそれぞれ個別に loadCardSettings を呼ぶこと）に依存しており、実装が変わった場合（例えば設定を一度だけ読み込んで共有するよう最適化された場合）に誤って壊れるテストになります。loadCardSettings の呼び出し回数ではなく、layout と displayOptions の値そのものをアサートする方がより堅牢です。
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/hooks/__tests__/useCardSettings.test.ts
Line: 43-46
Comment:
**実装の詳細に依存した脆弱なアサーション**
`loadCardSettings` が `useState` の遅延初期化で2回呼ばれることを `toHaveBeenCalledTimes(2)` でハードコードしています。これはフックの内部実装の詳細（2つの `useState` がそれぞれ個別に `loadCardSettings` を呼ぶこと）に依存しており、実装が変わった場合（例えば設定を一度だけ読み込んで共有するよう最適化された場合）に誤って壊れるテストになります。`loadCardSettings` の呼び出し回数ではなく、`layout` と `displayOptions` の値そのものをアサートする方がより堅牢です。
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
|
||
| // Now mount | ||
| rerender({ mounted: true }); | ||
|
|
||
| // Expect loadCardSettings to be called again during the effect (2 from init + 1 from effect) | ||
| expect(cardSettingsLib.loadCardSettings).toHaveBeenCalledTimes(3); | ||
| expect(result.current.layout).toEqual(mockDefaultLayout); | ||
| expect(result.current.displayOptions).toEqual(mockDefaultOptions); | ||
| }); | ||
|
|
||
| it("should not call saveCardSettings before hydration is complete", () => { | ||
| const { rerender } = renderHook( | ||
| ({ mounted }) => useCardSettings(mounted), |
There was a problem hiding this comment.
saveCardSettings 呼び出しが未検証
"should hydrate and update state when mounted becomes true" テストは loadCardSettings の呼び出し回数と状態値はアサートしていますが、ハイドレーション完了後（isHydrated が true になった後）に永続化エフェクトが saveCardSettings を呼ぶかどうかを検証していません。このサイドエフェクトは別テスト（"should not call saveCardSettings before hydration is complete"）でカバーされていますが、ハイドレーションテスト自体の観点としては不完全です。
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/hooks/__tests__/useCardSettings.test.ts
Line: 63-75
Comment:
**ハイドレーション後の `saveCardSettings` 呼び出しが未検証**
"should hydrate and update state when mounted becomes true" テストは `loadCardSettings` の呼び出し回数と状態値はアサートしていますが、ハイドレーション完了後（`isHydrated` が `true` になった後）に永続化エフェクトが `saveCardSettings` を呼ぶかどうかを検証していません。このサイドエフェクトは別テスト（"should not call saveCardSettings before hydration is complete"）でカバーされていますが、ハイドレーションテスト自体の観点としては不完全です。
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
🎯 What: Added tests for the
useCardSettingshook which handles local storage hydration, persistence, and state management for card layouts.📊 Coverage: Covered all paths including:
mountedprop transitions).saveCardSettingswhen state changes after hydration).toggleMainBlockVisibility,toggleDisplayOption).isBlockVisible).✨ Result: Improved test coverage and reliability for user-configured card settings, isolating internal behavior from local storage logic by effectively mocking dependent libraries.
PR created automatically by Jules for task 459312589284437376 started by @is0692vs
Greptile Summary
useCardSettingsフックに対するテストスイートを新規追加するPRです。ローカルストレージのハイドレーション、状態変更後の永続化、各モディファイア関数（toggleMainBlockVisibility・toggleDisplayOption）、isBlockVisibleヘルパーの動作を網羅しています。@ts-expect-errorディレクティブが複数あり（有効なCardBlockId値やkeyof CardDisplayOptions値に対して使用）、TypeScript 厳格チェック時にコンパイルエラーの原因になる可能性があります。loadCardSettingsの呼び出し回数をハードコードでアサートしている箇所が複数あり、実装の内部詳細に依存した脆弱なテストになっています。Confidence Score: 4/5
テスト専用ファイルの追加のみでプロダクションコードへの影響はなく、マージしてもアプリの動作は変わりません。
プロダクションコードを変更しないテストファイルの追加であり、マージ自体はリスクが低いです。ただし、不要な @ts-expect-error ディレクティブが TypeScript 厳格チェック時にコンパイルエラーを起こす可能性、重複テストや実装詳細への依存によるメンテナンスコストの増加など、テスト品質面での改善点が複数あります。
src/hooks/tests/useCardSettings.test.ts — @ts-expect-error の整理と重複テストの統合が望まれます。
Important Files Changed
Sequence Diagram
sequenceDiagram participant Test participant renderHook participant useCardSettings participant loadCardSettings participant saveCardSettings Test->>renderHook: "mounted=false" renderHook->>useCardSettings: init useCardSettings->>loadCardSettings: useState lazy init (layout) useCardSettings->>loadCardSettings: useState lazy init (options) Note over useCardSettings: isHydrated=false useCardSettings-->>renderHook: hydration effect skipped Test->>renderHook: "rerender(mounted=true)" renderHook->>useCardSettings: re-render useCardSettings->>loadCardSettings: hydration effect (3rd call) useCardSettings-->>useCardSettings: setIsHydrated(true) useCardSettings->>saveCardSettings: persistence effect firesComments Outside Diff (1)
src/hooks/__tests__/useCardSettings.test.ts, line 109-149 (link)@ts-expect-errorディレクティブが複数存在するtoggleMainBlockVisibility("profile")、toggleDisplayOption("showAvatar")、toggleDisplayOption("showBio")、isBlockVisible("profile")、isBlockVisible("stats")、setDisplayOptions({...})の呼び出しにそれぞれ@ts-expect-errorが付いていますが、これらは実際には型エラーにはなりません。"profile"および"stats"は有効なCardBlockId値であり、"showAvatar"および"showBio"は有効なkeyof CardDisplayOptions値です。TypeScript の厳格な設定下では、エラーのない行に@ts-expect-errorを付けると "Unused '@ts-expect-error' directive" というコンパイルエラーが発生します。なお、isBlockVisible("unknown")だけは"unknown"がCardBlockIdに含まれないため@ts-expect-errorが正当です。Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "test: fix typing issues in useCardSettin..." | Re-trigger Greptile