Conversation
|
WalkthroughThis pull request updates the Formbricks JavaScript SDK's setup configuration to support 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ 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. 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: 3
🤖 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/js/src/lib/load-formbricks.test.ts`:
- Around line 4-8: Replace the inline config shape with the canonical
TSetupConfig type: add a static import of TSetupConfig from the types module at
the top of the test file and change the setup declaration to use TSetupConfig
(i.e. let setup: (config: TSetupConfig) => Promise<void>), keeping the dynamic
import("./load-formbricks") behavior for runtime module reset unchanged so only
the type is resolved statically.
In `@packages/js/src/lib/load-formbricks.ts`:
- Around line 79-80: The parameter to validateSetupArgs should be typed as
TSetupConfig instead of unknown; change the signature from
validateSetupArgs(config: unknown) to validateSetupArgs(config: TSetupConfig),
remove the runtime cast (config as TSetupConfig) and destructure directly (const
{ appUrl, environmentId, workspaceId } = config), and ensure any callers (e.g.,
setup) already passing a TSetupConfig keep their types so the compiler surfaces
typos like enviromentId.
In `@packages/js/src/types/formbricks.ts`:
- Around line 6-13: Replace the inline setupConfig object in the
TFormbricks.setup signature with the existing TSetupConfig type to avoid
duplication; update the setup signature to accept (setupConfig: TSetupConfig) =>
Promise<void>, ensuring TSetupConfig (the type that includes environmentId,
workspaceId, appUrl and the `@deprecated` JSDoc) is referenced instead of
redefining the shape, and keep TSetupConfig declared in the same module (no
reordering required).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0eb613f6-4063-4367-8ebf-7fb00e8c2ae9
📒 Files selected for processing (5)
packages/js/README.mdpackages/js/src/index.test.tspackages/js/src/lib/load-formbricks.test.tspackages/js/src/lib/load-formbricks.tspackages/js/src/types/formbricks.ts
| let setup: (config: { | ||
| appUrl: string; | ||
| workspaceId?: string; | ||
| environmentId?: string; | ||
| }) => Promise<void>; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider importing TSetupConfig instead of redeclaring its shape.
This inline type mirrors TSetupConfig from packages/js/src/types/formbricks.ts. Since dynamic import("./load-formbricks") is used to reset module state, only the runtime value needs to be re-resolved — the type can be imported statically at the top of the file so it stays in sync with the source of truth.
♻️ Proposed change
import { afterEach, beforeEach, describe, expect, test, vi } from "vitest";
+import type { TSetupConfig } from "../types/formbricks";
// We need to import the module after each reset
-let setup: (config: {
- appUrl: string;
- workspaceId?: string;
- environmentId?: string;
-}) => Promise<void>;
+let setup: (config: TSetupConfig) => Promise<void>;
let callMethod: (method: string, ...args: unknown[]) => Promise<void>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/js/src/lib/load-formbricks.test.ts` around lines 4 - 8, Replace the
inline config shape with the canonical TSetupConfig type: add a static import of
TSetupConfig from the types module at the top of the test file and change the
setup declaration to use TSetupConfig (i.e. let setup: (config: TSetupConfig) =>
Promise<void>), keeping the dynamic import("./load-formbricks") behavior for
runtime module reset unchanged so only the type is resolved statically.
| const validateSetupArgs = (config: unknown): TSetupConfig | null => { | ||
| const { appUrl, environmentId, workspaceId } = config as TSetupConfig; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Tighten validateSetupArgs parameter typing.
config: unknown followed by config as TSetupConfig is strictly weaker than typing the parameter as TSetupConfig. The sole caller (setup, line 136) already constrains config to TSetupConfig, so the unknown + cast adds no runtime safety and removes compile-time safety (e.g., a typo like config.enviromentId would no longer surface).
♻️ Proposed change
-const validateSetupArgs = (config: unknown): TSetupConfig | null => {
- const { appUrl, environmentId, workspaceId } = config as TSetupConfig;
+const validateSetupArgs = (config: TSetupConfig): TSetupConfig | null => {
+ const { appUrl, environmentId, workspaceId } = config;📝 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.
| const validateSetupArgs = (config: unknown): TSetupConfig | null => { | |
| const { appUrl, environmentId, workspaceId } = config as TSetupConfig; | |
| const validateSetupArgs = (config: TSetupConfig): TSetupConfig | null => { | |
| const { appUrl, environmentId, workspaceId } = config; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/js/src/lib/load-formbricks.ts` around lines 79 - 80, The parameter
to validateSetupArgs should be typed as TSetupConfig instead of unknown; change
the signature from validateSetupArgs(config: unknown) to
validateSetupArgs(config: TSetupConfig), remove the runtime cast (config as
TSetupConfig) and destructure directly (const { appUrl, environmentId,
workspaceId } = config), and ensure any callers (e.g., setup) already passing a
TSetupConfig keep their types so the compiler surfaces typos like enviromentId.
| setup: (setupConfig: { | ||
| environmentId: string; | ||
| /** | ||
| * @deprecated use workspaceId instead, environmentId will be removed in a future version | ||
| */ | ||
| environmentId?: string; | ||
| workspaceId?: string; | ||
| appUrl: string; | ||
| }) => Promise<void>; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Deduplicate: reuse TSetupConfig in the setup signature.
The inline setupConfig shape here (lines 7-12) is a verbatim copy of the newly added TSetupConfig (lines 75-82), including the @deprecated JSDoc. The two definitions will drift (e.g., next time the deprecation wording or field set changes). Since TSetupConfig is declared in the same file, prefer referencing it directly.
♻️ Proposed refactor
export interface TFormbricks {
/**
* `@description` Initializes the Formbricks SDK.
* `@param` setupConfig - The configuration for the Formbricks SDK.
*/
- setup: (setupConfig: {
- /**
- * `@deprecated` use workspaceId instead, environmentId will be removed in a future version
- */
- environmentId?: string;
- workspaceId?: string;
- appUrl: string;
- }) => Promise<void>;
+ setup: (setupConfig: TSetupConfig) => Promise<void>;Note: this requires moving/hoisting the TSetupConfig declaration above TFormbricks, or adding a forward type alias — TypeScript allows type references inside an interface to resolve regardless of order in the same module.
📝 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.
| setup: (setupConfig: { | |
| environmentId: string; | |
| /** | |
| * @deprecated use workspaceId instead, environmentId will be removed in a future version | |
| */ | |
| environmentId?: string; | |
| workspaceId?: string; | |
| appUrl: string; | |
| }) => Promise<void>; | |
| setup: (setupConfig: TSetupConfig) => Promise<void>; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/js/src/types/formbricks.ts` around lines 6 - 13, Replace the inline
setupConfig object in the TFormbricks.setup signature with the existing
TSetupConfig type to avoid duplication; update the setup signature to accept
(setupConfig: TSetupConfig) => Promise<void>, ensuring TSetupConfig (the type
that includes environmentId, workspaceId, appUrl and the `@deprecated` JSDoc) is
referenced instead of redefining the shape, and keep TSetupConfig declared in
the same module (no reordering required).
1 participant
Adds support for
workspaceIdand keeps support forenvironmentIdas well for backwards compatibility by marking it as deprecated. Similar to the js-core changes for this