fix: set preferences data directly on fetch to prevent undefined on re-mount

[a827681306]

Summary

Fix usePreferences hook returning undefined for preferences when a component unmounts and re-mounts.

Root Cause

The fetchPreferences function was not calling setData() with the API response data. It relied solely on event listeners (preferences.list.updated, preferences.list.pending, preferences.list.resolved) to update the state via the sync callback.

On first mount, the event fires and sync sets the data correctly. However, when the component unmounts and re-mounts, the event listeners are re-registered but the cached response may not trigger a new event, leaving preferences as undefined permanently (with isLoading: false and no error).

Fix

Added setData(response.data) in the success branch of fetchPreferences, consistent with how useNotifications handles its fetch response. This ensures the data is always set directly from the API response, regardless of whether events fire.

Comparison with useNotifications

useNotifications correctly calls setData(responseData.notifications) in its fetchNotifications callback — this PR aligns usePreferences with that pattern.

Fixes #10057

[netlify]

👷 Deploy request for dashboard-v2-novu-staging pending review.

Visit the deploys page to approve it

Name	Link
🔨 Latest commit	9ffa0a6

[github-actions]

Hey there and thank you for opening this pull request! 👋

We require pull request titles to follow specific formatting rules and it looks like your proposed title needs to be adjusted.

Your PR title is: fix: set preferences data directly on fetch to prevent undefined on re-mount

Requirements:

1. Follow the Conventional Commits specification

Expected format: feat(scope): Add fancy new feature

Details:
No scope found in pull request title "fix: set preferences data directly on fetch to prevent undefined on re-mount". Scope must match one of: root, api-service, dashboard, inbound-mail, webhook, worker, ws, ee-api, ee-auth, ee-billing, ee-dal, ee-shared-services, ee-translation, application-generic, automation, dal, api, maily-core, maily-render, maily-tailwind-config, maily-tsconfig, notifications, testing, add-inbox, framework, js, nextjs, novu, providers, react, react-native, shared, stateless, nestjs, nextjs.

[coderabbitai]

Walkthrough

The change modifies the usePreferences hook to update local state when a successful response is received. Instead of unconditionally invoking the onSuccess callback, the code now checks for response.data existence and updates the local data state via setData before calling onSuccess. Error handling and other control flow remain unchanged. The modification does not alter any public API signatures.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main fix: setting preferences data directly on fetch to prevent undefined values on component re-mount.
Description check	✅ Passed	The description provides comprehensive context explaining the root cause, the fix, and how it aligns with useNotifications, all related to the changeset.
Linked Issues check	✅ Passed	The PR directly addresses the core requirement from issue #10057 by ensuring setData() is called in fetchPreferences success branch, fixing the undefined preferences state on re-mount.
Out of Scope Changes check	✅ Passed	The code changes are focused and scope-limited, only adding setData(response.data) call and adjusting the conditional branch logic without introducing unrelated modifications.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

packages/react/src/hooks/usePreferences.ts (1)

38-50: props.filter changes are silently ignored after mount.

The empty dependency array means the initial fetch and event-listener registration happen only once. If the consumer passes a dynamically-derived filter, the hook won't re-fetch when it changes. refetch() must be called manually, which is non-obvious.

♻️ Suggested approach: re-run effect when filter changes

  useEffect(() => {
    fetchPreferences();

    const listUpdatedCleanup = on('preferences.list.updated', sync);
    const listPendingCleanup = on('preferences.list.pending', sync);
    const listResolvedCleanup = on('preferences.list.resolved', sync);

    return () => {
      listUpdatedCleanup();
      listPendingCleanup();
      listResolvedCleanup();
    };
- }, []);
+ // eslint-disable-next-line react-hooks/exhaustive-deps
+ }, [JSON.stringify(props?.filter)]);

Serialising the filter avoids object-identity false positives while keeping the dep shallow.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/react/src/hooks/usePreferences.ts` around lines 38 - 50, The effect
in useEffect currently runs only once so changes to the incoming filter prop are
ignored; update the effect for useEffect that calls fetchPreferences and
registers event listeners (the block referencing fetchPreferences, on(..., sync)
and the cleanup functions
listUpdatedCleanup/listPendingCleanup/listResolvedCleanup) to re-run whenever
the external filter changes by including a stable serialized filter dependency
(e.g. JSON.stringify(filter) or another stable key for props.filter) in the
dependency array so fetchPreferences is called and listeners are
re-registered/cleaned up whenever filter changes.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/react/src/hooks/usePreferences.ts`:
- Around line 58-61: The current response handling in usePreferences (the
else-if branch around setData and onSuccess) misses the edge case where response
has neither data nor error, leaving consumers with undefined preferences; update
the response handling in usePreferences to add a defensive fallback: when
response.error is falsy and response.data is falsy, set an explicit error state
(e.g., setError(new Error("No data or error in response"))) and invoke any
onError callback (onError?.(new Error(...))) or otherwise set a sensible default
for preferences, ensuring isLoading/isFetching are still cleared; locate the
branching logic around setData(response.data) and onSuccess?.(response.data) and
add the fallback branch there.

---

Nitpick comments:
In `@packages/react/src/hooks/usePreferences.ts`:
- Around line 38-50: The effect in useEffect currently runs only once so changes
to the incoming filter prop are ignored; update the effect for useEffect that
calls fetchPreferences and registers event listeners (the block referencing
fetchPreferences, on(..., sync) and the cleanup functions
listUpdatedCleanup/listPendingCleanup/listResolvedCleanup) to re-run whenever
the external filter changes by including a stable serialized filter dependency
(e.g. JSON.stringify(filter) or another stable key for props.filter) in the
dependency array so fetchPreferences is called and listeners are
re-registered/cleaned up whenever filter changes.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Core fix is correct — setData now ensures state is populated directly from the API response.

The else if (response.data) guard is also an improvement over the old else { onSuccess?.(response.data!) } which used a non-null assertion that could silently pass undefined to the callback.

One edge case worth noting: if the API somehow returns a response where neither response.error nor response.data is set, neither branch executes, isLoading and isFetching still flip to false, and the consumer sees { preferences: undefined, error: undefined, isLoading: false, isFetching: false } — the same broken state described in issue #10057. Consider adding a fallback or at minimum an explicit error for this case:

🛡️ Defensive fallback for the no-data/no-error path

    } else if (response.data) {
      setData(response.data);
      onSuccess?.(response.data);
+   } else {
+     setError(new NovuError('Preferences response contained no data and no error.'));
    }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/react/src/hooks/usePreferences.ts` around lines 58 - 61, The current
response handling in usePreferences (the else-if branch around setData and
onSuccess) misses the edge case where response has neither data nor error,
leaving consumers with undefined preferences; update the response handling in
usePreferences to add a defensive fallback: when response.error is falsy and
response.data is falsy, set an explicit error state (e.g., setError(new
Error("No data or error in response"))) and invoke any onError callback
(onError?.(new Error(...))) or otherwise set a sensible default for preferences,
ensuring isLoading/isFetching are still cleared; locate the branching logic
around setData(response.data) and onSuccess?.(response.data) and add the
fallback branch there.