fix: click outside close for react-native sdk #6183
Conversation
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
WalkthroughThis change removes the localization string Possibly related PRs
Suggested labels
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/surveys/src/components/wrappers/survey-container.test.tsxOops! Something went wrong! :( ESLint: 8.57.0 ESLint couldn't find the plugin "eslint-plugin-turbo". (The package "eslint-plugin-turbo" was not found when loaded as a Node module from the directory "/packages/surveys".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-turbo" was referenced from the config file in "packages/surveys/.eslintrc.cjs » @formbricks/eslint-config/legacy-react.js » eslint-config-turbo". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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/surveys/src/components/wrappers/survey-container.test.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
`**/*.{tsx,jsx}`: Capture ref values in variables within useEffect cleanup Avoid accessing `.current` directly in cleanup functions
**/*.{tsx,jsx}: Capture ref values in variables within useEffect cleanup
Avoid accessing.currentdirectly in cleanup functions
📄 Source: CodeRabbit Inference Engine (.cursor/rules/build-and-deployment.mdc)
List of files the instruction was applied to:
packages/surveys/src/components/wrappers/survey-container.test.tsx
`**/*.test.{ts,tsx,js,jsx}`: Ensure all test mocks include required constants li...
**/*.test.{ts,tsx,js,jsx}: Ensure all test mocks include required constants likeSESSION_MAX_AGE
Remove unused imports and constants from test files
Use literal values instead of imported constants when the constant isn't actually needed
📄 Source: CodeRabbit Inference Engine (.cursor/rules/build-and-deployment.mdc)
List of files the instruction was applied to:
packages/surveys/src/components/wrappers/survey-container.test.tsx
`**/*.test.{tsx,jsx}`: Mock Next.js navigation hooks properly: `useParams`, `useRouter`, `useSearchParams`
**/*.test.{tsx,jsx}: Mock Next.js navigation hooks properly:useParams,useRouter,useSearchParams
📄 Source: CodeRabbit Inference Engine (.cursor/rules/build-and-deployment.mdc)
List of files the instruction was applied to:
packages/surveys/src/components/wrappers/survey-container.test.tsx
`**/*.tsx`: Search for `import.*[DeprecatedComponent]` patterns to identify depr...
**/*.tsx: Search forimport.*[DeprecatedComponent]patterns to identify deprecated component usage in the codebase
Exclude specified components (e.g., 'ModalWithTabs') from migration if the user lists them as exclusions
Update import statements to use the new component(s) as specified by the user
Transform component props according to user-specified mapping rules and remove deprecated props
Update the component structure to match the new component system, preserving all existing logic, state management, and event handlers
📄 Source: CodeRabbit Inference Engine (.cursor/rules/component-migration.mdc)
List of files the instruction was applied to:
packages/surveys/src/components/wrappers/survey-container.test.tsx
`**/*.test.{ts,tsx}`: Find and update corresponding test files (same name with `...
**/*.test.{ts,tsx}: Find and update corresponding test files (same name with.test.tsxor.test.ts) to mock the new component(s) and update test expectations
Replace old component mocks in test files with new component mocks as specified by the user
Update test IDs and component-specific assertions in test files to reflect the new component system
Mock all new component parts that appear in the migrated component within the corresponding test files
📄 Source: CodeRabbit Inference Engine (.cursor/rules/component-migration.mdc)
List of files the instruction was applied to:
packages/surveys/src/components/wrappers/survey-container.test.tsx
`**/*.{ts,tsx}`: Always scope queries by environmentId when accessing surveys or...
**/*.{ts,tsx}: Always scope queries by environmentId when accessing surveys or contacts to ensure proper environment-level data isolation.
When querying models that support soft deletion, always check the 'isActive' field and apply proper filtering in queries.
📄 Source: CodeRabbit Inference Engine (.cursor/rules/database.mdc)
List of files the instruction was applied to:
packages/surveys/src/components/wrappers/survey-container.test.tsx
`**/*.test.tsx`: For test files for ".tsx" files, add this code inside the "desc...
**/*.test.tsx: For test files for ".tsx" files, add this code inside the "describe" block and before any test: afterEach(() => { cleanup(); });
The "afterEach" function should only have the "cleanup()" line inside it and should be added to the "vitest" imports in test files for ".tsx" files
For click events in test files for ".tsx" files, import userEvent from "@testing-library/user-event"
You don't need to mock @tolgee/react in test files for ".tsx" files
Use "import "@testing-library/jest-dom/vitest";" in test files for ".tsx" files
📄 Source: CodeRabbit Inference Engine (.github/copilot-instructions.md)
List of files the instruction was applied to:
packages/surveys/src/components/wrappers/survey-container.test.tsx
🧠 Learnings (2)
📓 Common learnings
Learnt from: gupta-piyush19
PR: formbricks/formbricks#6104
File: apps/web/modules/survey/list/components/copy-survey-form.tsx:17-23
Timestamp: 2025-07-03T15:52:19.774Z
Learning: The CopySurveyForm component in apps/web/modules/survey/list/components/copy-survey-form.tsx does not require an onSurveysCopied callback prop, unlike other survey-related components in the same module that were updated to use this pattern.
Learnt from: gupta-piyush19
PR: formbricks/formbricks#5903
File: apps/web/modules/ui/components/alert-dialog/index.tsx:52-58
Timestamp: 2025-07-04T10:48:05.673Z
Learning: In the AlertDialog component (apps/web/modules/ui/components/alert-dialog/index.tsx), the team prefers that the onConfirm callback has full control over when to close the dialog, rather than automatically closing it after the callback executes. This maintains explicit control over dialog state management.
Learnt from: victorvhs017
PR: formbricks/formbricks#5694
File: packages/surveys/src/components/questions/date-question.test.tsx:90-99
Timestamp: 2025-05-07T15:08:05.358Z
Learning: In packages/surveys/src/components/questions tests, when testing back button clicks, expect setTtc to be called twice - once explicitly in the component's onClick handler and once through the useTtc hook implementation.
Learnt from: CR
PR: formbricks/formbricks#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-30T09:29:49.185Z
Learning: Applies to packages/survey/**/*.test.{js,ts,jsx,tsx} : If the file is located in the "packages/survey" path, use "@testing-library/preact" instead of "@testing-library/react"
Learnt from: pandeymangg
PR: formbricks/formbricks#6146
File: packages/surveys/src/lib/date-time.ts:54-78
Timestamp: 2025-07-03T07:50:43.192Z
Learning: The surveys package doesn't have a way to send/receive locale information, so date formatting functions in packages/surveys/src/lib/date-time.ts must use hardcoded "en-US" locale instead of making it configurable.
Learnt from: CR
PR: formbricks/formbricks#0
File: .cursor/rules/database.mdc:0-0
Timestamp: 2025-06-30T09:31:28.864Z
Learning: Applies to **/*.{ts,tsx} : Always scope queries by environmentId when accessing surveys or contacts to ensure proper environment-level data isolation.
Learnt from: CR
PR: formbricks/formbricks#0
File: .cursor/rules/component-migration.mdc:0-0
Timestamp: 2025-06-30T09:30:57.253Z
Learning: Applies to **/*.tsx : Exclude specified components (e.g., 'ModalWithTabs') from migration if the user lists them as exclusions
packages/surveys/src/components/wrappers/survey-container.test.tsx (18)
Learnt from: victorvhs017
PR: formbricks/formbricks#5694
File: packages/surveys/src/components/questions/date-question.test.tsx:90-99
Timestamp: 2025-05-07T15:08:05.358Z
Learning: In packages/surveys/src/components/questions tests, when testing back button clicks, expect setTtc to be called twice - once explicitly in the component's onClick handler and once through the useTtc hook implementation.
Learnt from: CR
PR: formbricks/formbricks#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-30T09:29:49.185Z
Learning: Applies to packages/survey/**/*.test.{js,ts,jsx,tsx} : If the file is located in the "packages/survey" path, use "@testing-library/preact" instead of "@testing-library/react"
Learnt from: victorvhs017
PR: formbricks/formbricks#5694
File: packages/surveys/src/components/questions/date-question.test.tsx:90-99
Timestamp: 2025-05-07T15:08:05.358Z
Learning: In the DateQuestion component tests, setTtc is called twice during back button click: once explicitly in the back button onClick handler and once in the useTtc hook's visibilitychange event handler.
Learnt from: CR
PR: formbricks/formbricks#0
File: .cursor/rules/component-migration.mdc:0-0
Timestamp: 2025-06-30T09:30:57.253Z
Learning: Applies to **/*.test.{ts,tsx} : Update test IDs and component-specific assertions in test files to reflect the new component system
Learnt from: CR
PR: formbricks/formbricks#0
File: .cursor/rules/component-migration.mdc:0-0
Timestamp: 2025-06-30T09:30:57.253Z
Learning: Applies to **/*.test.{ts,tsx} : Mock all new component parts that appear in the migrated component within the corresponding test files
Learnt from: gupta-piyush19
PR: formbricks/formbricks#6104
File: apps/web/modules/survey/list/components/copy-survey-form.tsx:17-23
Timestamp: 2025-07-03T15:52:19.774Z
Learning: The CopySurveyForm component in apps/web/modules/survey/list/components/copy-survey-form.tsx does not require an onSurveysCopied callback prop, unlike other survey-related components in the same module that were updated to use this pattern.
Learnt from: CR
PR: formbricks/formbricks#0
File: .cursor/rules/component-migration.mdc:0-0
Timestamp: 2025-06-30T09:30:57.253Z
Learning: Applies to **/*.test.{ts,tsx} : Find and update corresponding test files (same name with `.test.tsx` or `.test.ts`) to mock the new component(s) and update test expectations
Learnt from: CR
PR: formbricks/formbricks#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-30T09:29:49.185Z
Learning: Applies to **/*.test.tsx : For click events in test files for ".tsx" files, import userEvent from "@testing-library/user-event"
Learnt from: CR
PR: formbricks/formbricks#0
File: .cursor/rules/component-migration.mdc:0-0
Timestamp: 2025-06-30T09:30:57.253Z
Learning: Applies to **/*.test.{ts,tsx} : Replace old component mocks in test files with new component mocks as specified by the user
Learnt from: CR
PR: formbricks/formbricks#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-30T09:29:49.185Z
Learning: Applies to **/*.test.tsx : For test files for ".tsx" files, add this code inside the "describe" block and before any test: afterEach(() => { cleanup(); });
Learnt from: CR
PR: formbricks/formbricks#0
File: .cursor/rules/testing-patterns.mdc:0-0
Timestamp: 2025-06-23T12:24:51.712Z
Learning: Test multiple scenarios in hooks (such as different modes or parameters) by changing mock return values and rerendering.
Learnt from: CR
PR: formbricks/formbricks#0
File: .cursor/rules/testing-patterns.mdc:0-0
Timestamp: 2025-06-23T12:24:51.712Z
Learning: Always test cleanup logic in hooks by verifying that resources (such as AbortController) are properly cleaned up on unmount.
Learnt from: CR
PR: formbricks/formbricks#0
File: .cursor/rules/testing-patterns.mdc:0-0
Timestamp: 2025-06-23T12:24:51.712Z
Learning: For comprehensive hook test coverage, include tests for initialization, data fetching (success and error), state updates, dependency changes, manual actions, race conditions, cleanup, mode switching, and edge cases.
Learnt from: CR
PR: formbricks/formbricks#0
File: .cursor/rules/react-context-patterns.mdc:0-0
Timestamp: 2025-06-23T12:24:31.379Z
Learning: For context-based tests in React, mock dependencies such as `next/navigation`'s `useParams`, response filter contexts, and any API actions the provider depends on to isolate test behavior.
Learnt from: CR
PR: formbricks/formbricks#0
File: .cursor/rules/testing-patterns.mdc:0-0
Timestamp: 2025-06-23T12:24:51.712Z
Learning: To test useEffect dependencies in hooks, change mock return values and trigger rerender to simulate dependency changes.
Learnt from: CR
PR: formbricks/formbricks#0
File: .cursor/rules/testing-patterns.mdc:0-0
Timestamp: 2025-06-23T12:24:51.712Z
Learning: For async React hook tests, always use waitFor to handle asynchronous state changes, and verify both loading and completed states as well as API call parameters.
Learnt from: CR
PR: formbricks/formbricks#0
File: .cursor/rules/testing-patterns.mdc:0-0
Timestamp: 2025-06-23T12:24:51.712Z
Learning: When testing React hooks that use React Context, mock the context values and functions using vi.mocked to ensure predictable test behavior.
Learnt from: CR
PR: formbricks/formbricks#0
File: .cursor/rules/react-context-patterns.mdc:0-0
Timestamp: 2025-06-23T12:24:31.379Z
Learning: When testing components that consume React context, always wrap them in the appropriate provider and use props like `initialCount` to ensure predictable test scenarios.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Run E2E Tests / Run E2E Tests
- GitHub Check: Build Formbricks-web / Build Formbricks-web
- GitHub Check: Run Linters / Linters
- GitHub Check: Run Unit Tests / Unit Tests
- GitHub Check: Cursor BugBot
- GitHub Check: Validate Docker Build
- GitHub Check: SonarQube
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
packages/surveys/src/components/wrappers/survey-container.test.tsx (1)
213-226: Test implementation looks correct for the new functionality.This test properly verifies that when
ignorePlacementForClickOutsideis true and placement is not "center", clicking outside triggers theonClosecallback. The test correctly usesdocument.bodyas the click target to simulate a general outside click.
| test("does not trigger clickOutside logic if ignorePlacementForClickOutside is true and placement is center", () => { | ||
| render( | ||
| <SurveyContainer | ||
| mode="modal" | ||
| placement="center" | ||
| clickOutside={false} | ||
| onClose={onCloseMock} | ||
| ignorePlacementForClickOutside={true}> | ||
| {(<TestChild />) as any} | ||
| </SurveyContainer> | ||
| ); | ||
| fireEvent.mouseDown(document.body); | ||
| expect(onCloseMock).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Test name is misleading - clarify what behavior is being tested.
The test name suggests it's testing the ignorePlacementForClickOutside logic, but the actual test has clickOutside={false}, which means no click outside logic should be triggered regardless of other props. This makes the test name confusing.
Consider renaming the test to better reflect what it's actually testing:
- test("does not trigger clickOutside logic if ignorePlacementForClickOutside is true and placement is center", () => {
+ test("does not trigger clickOutside logic when clickOutside is false regardless of ignorePlacementForClickOutside", () => {Alternatively, if you want to test the specific behavior with placement="center" and ignorePlacementForClickOutside=true, consider adding a separate test with clickOutside={true} to verify the expected behavior.
📝 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.
| test("does not trigger clickOutside logic if ignorePlacementForClickOutside is true and placement is center", () => { | |
| render( | |
| <SurveyContainer | |
| mode="modal" | |
| placement="center" | |
| clickOutside={false} | |
| onClose={onCloseMock} | |
| ignorePlacementForClickOutside={true}> | |
| {(<TestChild />) as any} | |
| </SurveyContainer> | |
| ); | |
| fireEvent.mouseDown(document.body); | |
| expect(onCloseMock).not.toHaveBeenCalled(); | |
| }); | |
| test("does not trigger clickOutside logic when clickOutside is false regardless of ignorePlacementForClickOutside", () => { | |
| render( | |
| <SurveyContainer | |
| mode="modal" | |
| placement="center" | |
| clickOutside={false} | |
| onClose={onCloseMock} | |
| ignorePlacementForClickOutside={true}> | |
| {(<TestChild />) as any} | |
| </SurveyContainer> | |
| ); | |
| fireEvent.mouseDown(document.body); | |
| expect(onCloseMock).not.toHaveBeenCalled(); | |
| }); |
🤖 Prompt for AI Agents
In packages/surveys/src/components/wrappers/survey-container.test.tsx around
lines 228 to 241, the test name is misleading because it implies testing
ignorePlacementForClickOutside behavior, but clickOutside is set to false,
disabling click outside logic entirely. Rename the test to reflect that it
verifies no clickOutside logic triggers when clickOutside is false, or add a new
test with clickOutside set to true and placement center to specifically test
ignorePlacementForClickOutside behavior.
|
|
We are going to postpone the changes to a later point in time and first focus on making the current SDKs stable while focussing on Formbricks 5. |
What does this PR do?
This is a companion PR to the formbricks/react-native#16 PR which adds the prop to the surveys package for ignoring the placement settings and enabling click outside close
How should this be tested?
Checklist
Required
pnpm buildconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
Bug Fixes
Chores