Skip to content

🧹 [code health] Refactor useCardSettings to remove redundant effect and eslint-disable #376

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

Open
is0692vs wants to merge 1 commit into
base: main
Choose a base branch
from
+2 −19
Open

🧹 [code health] Refactor useCardSettings to remove redundant effect and eslint-disable #376

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
21 changes: 2 additions & 19 deletions src/hooks/useCardSettings.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,36 +10,19 @@ import {
import type { CardLayout, CardBlockId } from "@/lib/types";

export function useCardSettings(mounted: boolean) {
const [isHydrated, setIsHydrated] = useState(false);
const [layout, setLayout] = useState<CardLayout>(() => loadCardSettings().layout);
const [displayOptions, setDisplayOptions] = useState<CardDisplayOptions>(
() => loadCardSettings().options,
);
Comment on lines 13 to 16

@greptile-apps greptile-apps Bot Jun 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 loadCardSettings() が2つの useState 遅延初期化で個別に呼ばれており、localStorage への読み取りが2回発生します。1回の呼び出しに統一することで、I/Oを減らしつつコードの意図も明確になります。

Suggested change
const [layout, setLayout] = useState<CardLayout>(() => loadCardSettings().layout);
const [displayOptions, setDisplayOptions] = useState<CardDisplayOptions>(
() => loadCardSettings().options,
);
const [layout, setLayout] = useState<CardLayout>(() => {
const { layout } = loadCardSettings();
return layout;
});
const [displayOptions, setDisplayOptions] = useState<CardDisplayOptions>(() => {
const { options } = loadCardSettings();
return options;
});
Prompt To Fix With AI 
This is a comment left during a code review.
Path: src/hooks/useCardSettings.ts
Line: 13-16

Comment:
`loadCardSettings()` が2つの `useState` 遅延初期化で個別に呼ばれており、localStorage への読み取りが2回発生します。1回の呼び出しに統一することで、I/Oを減らしつつコードの意図も明確になります。

```suggestion
  const [layout, setLayout] = useState<CardLayout>(() => {
    const { layout } = loadCardSettings();
    return layout;
  });
  const [displayOptions, setDisplayOptions] = useState<CardDisplayOptions>(() => {
    const { options } = loadCardSettings();
    return options;
  });
```

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!


// Initialize state from storage on mount
useEffect(() => {
if (!mounted || isHydrated) {
return;
}

const { layout: storedLayout, options: storedOptions } = loadCardSettings();

// eslint-disable-next-line react-hooks/set-state-in-effect
setLayout((prev) => JSON.stringify(prev) !== JSON.stringify(storedLayout) ? storedLayout : prev);
// eslint-disable-next-line react-hooks/set-state-in-effect
setDisplayOptions((prev) => JSON.stringify(prev) !== JSON.stringify(storedOptions) ? storedOptions : prev);

setIsHydrated(true);
}, [mounted, isHydrated]);

// Persist changes to storage
useEffect(() => {
if (!mounted || !isHydrated) {
if (!mounted) {
return;
}

saveCardSettings(layout, displayOptions);
}, [layout, displayOptions, mounted, isHydrated]);
}, [layout, displayOptions, mounted]);
Comment on lines 13 to +25

@gemini-code-assist gemini-code-assist Bot Jun 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

critical

🚨 Critical Bug: User settings will be wiped out on every page load (SSR/Hydration issue)

While removing the isHydrated state and the useEffect seems like a clean refactoring, it introduces a critical bug that completely overwrites and wipes out the user's saved settings in localStorage with the default settings on every page load.

Why this happens:

  1. Rules of Hooks: In any parent component (like CardGeneratorModal), hooks must be called at the top level, before any early returns (such as if (!mounted) return null;).
  2. Server-Side Rendering (SSR): During SSR, mounted is false. useCardSettings(false) is executed on the server. Since window is undefined, loadCardSettings() returns the default settings, and the state (layout and displayOptions) is initialized to these defaults.
  3. Client-Side Hydration: During hydration, mounted is still false. The state remains initialized to the default values.
  4. Mounting: Once the component mounts, mounted becomes true.
  5. The Bug: The persistence useEffect triggers because mounted changed to true. Since the isHydrated guard was removed, the effect immediately calls saveCardSettings(layout, displayOptions) using the current state values—which are the default values!
  6. Data Loss: This immediately overwrites the user's custom settings in localStorage with the defaults.

Solution:

To prevent this, we must keep the isHydrated guard (or a similar mechanism) to ensure we never write to localStorage until we have successfully loaded the stored settings on the client side after mounting.

  const [layout, setLayout] = useState<CardLayout>(() => loadCardSettings().layout);
  const [displayOptions, setDisplayOptions] = useState<CardDisplayOptions>(
    () => loadCardSettings().options,
  );
  const [isHydrated, setIsHydrated] = useState(false);

  // Initialize state from storage on mount
  useEffect(() => {
    if (!mounted || isHydrated) {
      return;
    }

    const { layout: storedLayout, options: storedOptions } = loadCardSettings();
    setLayout(storedLayout);
    setDisplayOptions(storedOptions);
    setIsHydrated(true);
  }, [mounted, isHydrated]);

  // Persist changes to storage
  useEffect(() => {
    if (!mounted || !isHydrated) {
      return;
    }

    saveCardSettings(layout, displayOptions);
  }, [layout, displayOptions, mounted, isHydrated]);


const toggleMainBlockVisibility = useCallback((blockId: CardBlockId) => {
setLayout((prev) => toggleBlockVisibility(prev, blockId));
Expand Down
Loading