Conversation
WalkthroughAdds a root Pre-merge checks✅ Passed checks (3 passed)
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: 7
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
package.json(1 hunks)packages/js/.eslintrc.cjs(0 hunks)packages/js/eslint.config.mjs(1 hunks)packages/js/package.json(1 hunks)packages/js/src/index.test.ts(13 hunks)packages/js/src/index.ts(1 hunks)packages/js/src/lib/load-formbricks.test.ts(14 hunks)packages/js/src/lib/load-formbricks.ts(4 hunks)packages/js/src/types/formbricks.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/js/.eslintrc.cjs
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-24T17:04:00.743Z
Learnt from: pandeymangg
PR: formbricks/js#6
File: packages/js/src/lib/load-formbricks.test.ts:96-114
Timestamp: 2025-09-24T17:04:00.743Z
Learning: The formbricks/js project uses happy-dom as the test environment in Vitest configuration, not jsdom. This is configured in packages/js/vite.config.ts and works well for DOM-dependent tests.
Applied to files:
packages/js/src/lib/load-formbricks.test.ts
📚 Learning: 2025-09-24T17:04:00.743Z
Learnt from: pandeymangg
PR: formbricks/js#6
File: packages/js/src/lib/load-formbricks.test.ts:96-114
Timestamp: 2025-09-24T17:04:00.743Z
Learning: The formbricks/js project uses happy-dom as the test environment in Vitest configuration (packages/js/vite.config.ts), not jsdom. This configuration works well for DOM-dependent tests and is faster than jsdom.
Applied to files:
packages/js/src/lib/load-formbricks.test.ts
📚 Learning: 2025-09-24T17:03:14.148Z
Learnt from: pandeymangg
PR: formbricks/js#6
File: packages/js/src/index.test.ts:27-103
Timestamp: 2025-09-24T17:03:14.148Z
Learning: In packages/js/src/index.test.ts, the maintainer prefers individual test functions for proxy-forwarding tests rather than table-driven tests, valuing explicit and independently readable test cases.
Applied to files:
packages/js/src/index.test.ts
🧬 Code graph analysis (4)
packages/js/src/lib/load-formbricks.test.ts (2)
packages/js/src/lib/load-formbricks.ts (1)
loadFormbricksToProxy(133-147)packages/js/src/types/formbricks.ts (1)
TFormbricks(1-35)
packages/js/src/index.ts (2)
packages/js/src/lib/load-formbricks.ts (1)
loadFormbricksToProxy(133-147)packages/js/src/types/formbricks.ts (1)
TFormbricks(1-35)
packages/js/src/index.test.ts (1)
packages/js/src/types/formbricks.ts (1)
TFormbricks(1-35)
packages/js/src/lib/load-formbricks.ts (1)
packages/js/src/types/formbricks.ts (1)
TFormbricks(1-35)
🔇 Additional comments (10)
packages/js/src/index.ts (1)
17-17: LGTM: Type narrowing improves safety.Casting
proptokeyof TFormbricksstrengthens type checking at the proxy boundary, ensuring only valid TFormbricks method names are forwarded toloadFormbricksToProxy.packages/js/src/types/formbricks.ts (1)
29-29: LGTM: Formatting consistency.The trailing comma aligns with the coding style used throughout the PR.
packages/js/src/lib/load-formbricks.test.ts (3)
5-8: LGTM: Strengthened test type safety.Constraining
loadFormbricksToProxyto acceptkeyof TFormbricksaligns the test helper with the typed proxy implementation and catches invalid method names at compile time.
23-24: LGTM: Cleaner global access.The
typedGlobalThisalias eliminates repeated type assertions and improves readability throughout the test file.
82-89: Verify the @ts-expect-error is still needed.The inline implementation with
@ts-expect-errorsuggests a type mismatch between the mock signature and Node's actualsetTimeout. Since this is test-only code and the mock works correctly, the suppression is acceptable, but ensure it remains necessary as TypeScript or Vitest types evolve.packages/js/src/index.test.ts (2)
10-10: LGTM: Type import supports stronger testing.Importing
TFormbricksenables the typedcustomMethodtest and better compile-time validation of proxy method calls.
151-155: LGTM: Typed custom method test.Using
keyof TFormbricksand accessingargs[0]properly constrains the test to valid method names while avoiding arbitrary argument spreading.packages/js/src/lib/load-formbricks.ts (3)
4-5: LGTM: Typed global declaration.Changing to
constwithTFormbricks & Record<string, (...args: unknown[]) => unknown>improves type safety while allowing the SDK to expose additional methods beyond the core interface.
71-85: LGTM: Strengthened parameter type.Changing
formbricksInstancefromanytoTFormbricksimproves type safety and makes the method validation check (lines 73-77) more explicit.
108-109: LGTM: Appropriate suppression for dynamic dispatch.The
@ts-expect-errorandeslint-disablecomments are necessary for runtime dynamic method invocation, which cannot be fully type-checked statically.
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/js/package.json (1)
52-53: Add Prettier to satisfy eslint-plugin-prettier’s peer dependency.
This was flagged earlier and still applies: without a localprettierdependency, pnpm will error when resolvingeslint-plugin-prettier@5. Please add it here."eslint-plugin-prettier": "5.5.4", + "prettier": "3.6.2",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
apps/playground/package.json(1 hunks)package.json(1 hunks)packages/js/eslint.config.mjs(1 hunks)packages/js/package.json(1 hunks)packages/js/src/index.test.ts(13 hunks)turbo.json(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-24T16:57:19.813Z
Learnt from: pandeymangg
PR: formbricks/js#6
File: turbo.json:20-23
Timestamp: 2025-09-24T16:57:19.813Z
Learning: Package-scoped task names like "formbricks/js:test" are valid and functional syntax in Turborepo's root turbo.json configuration, contrary to previous understanding that task keys must be plain task names.
Applied to files:
turbo.json
🧬 Code graph analysis (1)
packages/js/src/index.test.ts (1)
packages/js/src/types/formbricks.ts (1)
TFormbricks(1-35)
🔇 Additional comments (4)
packages/js/src/index.test.ts (4)
10-10: Good addition for type safety.The
TFormbricksimport enables type-safe dynamic method invocation in the test at line 150, replacing previousanycasts with proper type constraints.
140-140: Past review comment addressed.The stale comment about casting to any has been removed as requested in the previous review.
150-153: Type safety improvement looks good.Replacing the previous
anycast withkeyof TFormbricksconstraint provides compile-time safety while still testing dynamic method invocation through the proxy.
163-163: Appropriate mock type cast.The
as unknown as voidcast is necessary here becauseloadFormbricksToProxyreturnsPromise<void>, but this test verifies that the proxy correctly propagates non-void return values. The double cast is the correct way to override TypeScript's type checking for testing purposes.
2 participants
Adds eslint 9 support for the js package and removes the vercel style guide package