feat: add onSetup Prop to the Formbricks component #24
Conversation
|
Mohammed Emad seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
WalkthroughAn optional onSetup callback prop was added to the React Native Formbricks component. The FormbricksProps interface now includes onSetup?: () => void, and the Formbricks function signature destructures onSetup from props. After a successful asynchronous setup, the component invokes onSetup if it is provided. Error handling and the existing survey subscription/rendering logic remain unchanged. No other functional paths were modified. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
packages/react-native/src/components/formbricks.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/react-native/src/components/formbricks.tsx (3)
packages/react-native/src/lib/survey/store.ts (2)
SurveyStore(5-44)subscribe(39-43)packages/react-native/src/lib/common/logger.ts (2)
logger(23-35)Logger(8-48)packages/react-native/src/lib/common/setup.ts (1)
setup(277-357)
🔇 Additional comments (4)
packages/react-native/src/components/formbricks.tsx (4)
8-10: Prop shape looks good and remains backward compatible.Optional callback and naming are clear.
16-20: Signature change OK; dependency choice is intentional.Excluding onSetup from deps avoids re-running setup when only the callback identity changes.
If you expect onSetup to change and still want the latest callback invoked without re-running setup, I can provide a ref-based pattern.
41-44: subscribe wrapper is correct for useSyncExternalStore.Clean unsubscribe flow; no issues.
46-49: Snapshot accessor is correct; keep arrow to preserve this binding.Returning null to render nothing is fine.
| // initializes sdk | ||
| useEffect(() => { | ||
| const setupFormbricks = async (): Promise<void> => { | ||
| try { | ||
| await setup({ | ||
| environmentId, | ||
| appUrl, | ||
| }); | ||
|
|
||
| onSetup?.(); | ||
| } catch { | ||
| logger.debug("Initialization failed"); | ||
| } | ||
| }; | ||
|
|
||
| setupFormbricks().catch(() => { | ||
| logger.debug("Initialization error"); | ||
| }); | ||
| }, [environmentId, appUrl]); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
🛠️ Refactor suggestion
onSetup fires even when setup fails; also weak error handling and duplicate catch.
setup returns a Result, not exceptions, so onSetup currently runs on failures too. Catch blocks log at debug (likely invisible with default log level), drop the original error, and the outer .catch is redundant.
Apply:
useEffect(() => {
const setupFormbricks = async (): Promise<void> => {
try {
- await setup({
+ const result = await setup({
environmentId,
appUrl,
});
-
- onSetup?.();
- } catch {
- logger.debug("Initialization failed");
+ if (result.ok) {
+ onSetup?.();
+ } else {
+ logger.error(
+ `Initialization failed: ${result.error?.message ?? String(result.error)}`
+ );
+ }
+ } catch (err) {
+ logger.error(
+ `Initialization threw: ${
+ err instanceof Error ? err.message : String(err)
+ }`
+ );
}
};
- setupFormbricks().catch(() => {
- logger.debug("Initialization error");
- });
+ void setupFormbricks();
}, [environmentId, appUrl]);I can also wire error details into an analytics/log pipeline if you share the preferred channel.
📝 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.
| // initializes sdk | |
| useEffect(() => { | |
| const setupFormbricks = async (): Promise<void> => { | |
| try { | |
| await setup({ | |
| environmentId, | |
| appUrl, | |
| }); | |
| onSetup?.(); | |
| } catch { | |
| logger.debug("Initialization failed"); | |
| } | |
| }; | |
| setupFormbricks().catch(() => { | |
| logger.debug("Initialization error"); | |
| }); | |
| }, [environmentId, appUrl]); | |
| // initializes sdk | |
| useEffect(() => { | |
| const setupFormbricks = async (): Promise<void> => { | |
| try { | |
| const result = await setup({ | |
| environmentId, | |
| appUrl, | |
| }); | |
| if (result.ok) { | |
| onSetup?.(); | |
| } else { | |
| logger.error( | |
| `Initialization failed: ${result.error?.message ?? String(result.error)}` | |
| ); | |
| } | |
| } catch (err) { | |
| logger.error( | |
| `Initialization threw: ${ | |
| err instanceof Error ? err.message : String(err) | |
| }` | |
| ); | |
| } | |
| }; | |
| void setupFormbricks(); | |
| }, [environmentId, appUrl]); |
There was a problem hiding this comment.
✅ Thanks for the review! I’ve updated the implementation as suggested:
onSetup now only runs when setup succeeds (result.ok).
Replaced unsafe error?.message access with String(error) to satisfy TS union types.
Improved error logging with preserved details.
There was a problem hiding this comment.
@omdam Perfect! 🎉 I can see you've implemented all the suggested improvements correctly:
✅ Conditional callback execution - onSetup?.() now only runs when result.ok is true
✅ Type-safe error handling - Using String(result.error) for union type safety
✅ Enhanced error logging - Proper error details preservation with appropriate log levels
✅ Clean async handling - Removed the redundant .catch() and used void setupFormbricks()
The implementation now properly handles both success and failure scenarios, ensuring the onSetup callback only fires when the Formbricks SDK is actually ready. Great work on addressing all the feedback!
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
- Replaced unsafe error?.message access with String(error) to satisfy TS union types - Ensured onSetup only runs when setup succeeds - Improved error logging to preserve details and removed redundant catch
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
packages/react-native/src/components/formbricks.tsx (2)
22-47: Remove redundant outer .catch and add cancellation to avoid stale callbacks after unmount.The inner try/catch already handles errors. The extra .catch only logs at debug and is redundant. Also guard against invoking onSetup after unmount or when deps change mid-flight.
// initializes sdk useEffect(() => { + let cancelled = false; const setupFormbricks = async (): Promise<void> => { try { const result = await setup({ environmentId, appUrl, }); + if (cancelled) return; if (result.ok) { - onSetup?.(); + onSetup?.(); } else { logger.error(`Initialization failed: ${String(result.error)}`); } } catch (err) { - logger.error( - `Initialization threw: ${ - err instanceof Error ? err?.message : String(err) - }` - ); + if (!cancelled) { + logger.error( + `Initialization threw: ${ + err instanceof Error ? err.message : String(err) + }` + ); + } } }; - setupFormbricks().catch(() => { - logger.debug("Initialization error"); - }); + void setupFormbricks(); + return () => { + cancelled = true; + }; }, [environmentId, appUrl]);
37-40: Minor: drop unnecessary optional chaining on Error.message.err is confirmed Error in this branch; err.message is sufficient.
- `Initialization threw: ${ - err instanceof Error ? err?.message : String(err) - }` + `Initialization threw: ${ + err instanceof Error ? err.message : String(err) + }`
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
packages/react-native/src/components/formbricks.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/react-native/src/components/formbricks.tsx (4)
packages/react-native/src/lib/survey/store.ts (2)
SurveyStore(5-44)subscribe(39-43)packages/react-native/src/lib/common/logger.ts (2)
logger(23-35)Logger(8-48)packages/react-native/src/lib/common/setup.ts (1)
setup(277-357)packages/react-native/src/types/error.ts (1)
err(17-20)
🔇 Additional comments (1)
packages/react-native/src/components/formbricks.tsx (1)
7-11: Prop addition looks good and is backward compatible.Optional onSetup?: () => void extends the API safely without breaking existing consumers.
| // initializes sdk | ||
| useEffect(() => { | ||
| const setupFormbricks = async (): Promise<void> => { |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Keep the latest onSetup without retriggering setup; also prevent duplicate calls per env/app pair.
React 18 StrictMode can double-invoke effects in dev; setup() may also return ok when already initialized. Use refs to (a) always call the latest onSetup and (b) ensure it fires once per (environmentId, appUrl).
export function Formbricks({
appUrl,
environmentId,
onSetup,
}: FormbricksProps): React.JSX.Element | null {
- // initializes sdk
+ // use latest onSetup and guard against duplicate calls per (envId|appUrl)
+ const onSetupRef = React.useRef(onSetup);
+ useEffect(() => {
+ onSetupRef.current = onSetup;
+ }, [onSetup]);
+ const onSetupKeyRef = React.useRef<string | null>(null);
+
+ // initializes sdk
useEffect(() => {
+ const key = `${environmentId}|${appUrl}`;
const setupFormbricks = async (): Promise<void> => {
try {
const result = await setup({
environmentId,
appUrl,
});
- if (result.ok) {
- onSetup?.();
+ if (result.ok && onSetupKeyRef.current !== key) {
+ onSetupKeyRef.current = key;
+ onSetupRef.current?.();
} else {
logger.error(`Initialization failed: ${String(result.error)}`);
}
} catch (err) {
logger.error(Also applies to: 30-31
🤖 Prompt for AI Agents
In packages/react-native/src/components/formbricks.tsx around lines 21-23 (also
applies to 30-31), the effect that calls setup() should use refs to avoid stale
callbacks and to prevent duplicate initializations per environmentId/appUrl:
store the latest onSetup in a useRef and update it on each render so the effect
always invokes the newest callback; maintain a ref (or Map) keyed by
`${environmentId}:${appUrl}` to track which pairs have already been initialized
and skip calling setup if the key exists; ensure the effect uses minimal
dependencies (environmentId and appUrl) to run when those change and call the
setup routine only when the tracked key is not present, marking the key as
initialized immediately before/after successful setup to prevent double calls
under React StrictMode.
| if (result.ok) { | ||
| onSetup?.(); | ||
| } else { | ||
| logger.error(`Initialization failed: ${String(result.error)}`); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Prefer robust error message extraction to avoid "object Object".
Log the message when present; fall back to stringifying the error.
- } else {
- logger.error(`Initialization failed: ${String(result.error)}`);
- }
+ } else {
+ const msg =
+ (result as any)?.error?.message ?? JSON.stringify(result.error);
+ logger.error(`Initialization failed: ${msg}`);
+ }📝 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.
| if (result.ok) { | |
| onSetup?.(); | |
| } else { | |
| logger.error(`Initialization failed: ${String(result.error)}`); | |
| } | |
| if (result.ok) { | |
| onSetup?.(); | |
| } else { | |
| const msg = | |
| (result as any)?.error?.message ?? JSON.stringify(result.error); | |
| logger.error(`Initialization failed: ${msg}`); | |
| } |
🤖 Prompt for AI Agents
In packages/react-native/src/components/formbricks.tsx around lines 30 to 34,
the error logging may produce "[object Object]" because it blindly stringifies
the error; update the logger call to extract a clear message by using
result.error?.message when available, otherwise use the error if it's a string,
and as a last resort JSON.stringify(result.error) or result.error.toString();
replace the existing logger.error call with this prioritized extraction so logs
show useful error text.
This PR introduces a new optional prop onSetup to the Formbricks React Native component.
The onSetup callback is executed once the SDK initialization (init) is completed successfully. This allows developers to trigger custom logic on mobile apps after ensuring that Formbricks is fully set up.
Motivation
On mobile, certain flows require confirmation that the SDK is ready before proceeding (e.g., starting surveys, syncing data, or updating UI state). Without this hook, there was no straightforward way to guarantee that init had finished.
Changes
Added a new prop:
onSetup?: () => void;
Invoked onSetup after successful init completion in the setup flow.
Example Usage
<Formbricks
appUrl={APP_URL}
environmentId={ENV_ID}
onSetup={() => {
console.log("Formbricks initialized, ready to run custom logic");
}}
/>
Impact
Backwards compatible (prop is optional).
Enables mobile developers to reliably run code after Formbricks initialization.