feat: adds workspace support and environmentId backwards compatibility#59
Open
pandeymangg wants to merge 2 commits intoepic/v5from
Open
feat: adds workspace support and environmentId backwards compatibility#59pandeymangg wants to merge 2 commits intoepic/v5from
pandeymangg wants to merge 2 commits intoepic/v5from
Conversation
WalkthroughThis pull request migrates the React Native Formbricks SDK from an environment-centric to workspace-centric architecture. The change involves renaming configuration types ( 🚥 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: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/react-native/src/lib/common/tests/__mocks__/config.mock.ts (1)
87-98:⚠️ Potential issue | 🟡 MinorAlign the
settingsmock withTWorkspaceStateSettings.Line 90 still uses
darkOverlay, but the new workspace settings type expectsoverlay. Theas unknown as TConfigcast masks the mismatch, so tests can pass with a shape production code should not receive.🛠️ Proposed fix
settings: { recontactDays: 14, clickOutsideClose: true, - darkOverlay: false, + overlay: false, placement: "bottomRight", inAppSurveyBranding: true,As per coding guidelines,
**/*.{ts,tsx}: Use TypeScript with strict compiler settings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-native/src/lib/common/tests/__mocks__/config.mock.ts` around lines 87 - 98, The mock settings in the config mock use the old property darkOverlay which no longer matches TWorkspaceStateSettings (now expects overlay); update the settings object in packages/react-native/src/lib/common/tests/__mocks__/config.mock.ts to replace darkOverlay with overlay (preserving the boolean value) and ensure the overall shape matches TWorkspaceStateSettings so you can remove or narrow the unsafe "as unknown as TConfig" cast; check the settings object and the nested styling block and adjust types to align with TWorkspaceStateSettings and TConfig so the test mock reflects production types.packages/react-native/src/lib/common/config.ts (1)
98-115:⚠️ Potential issue | 🟠 MajorPersist migrated legacy config on first successful load.
migrateLegacyConfigonly updates the in-memory value. Legacy AsyncStorage entries withenvironmentId/environmentremain unchanged, so the “first run” migration never actually writes the new workspace shape unless a laterupdate()happens.💾 Proposed fix
const savedConfig = await AsyncStorage.getItem(RN_ASYNC_STORAGE_KEY); if (savedConfig) { + const rawConfig = JSON.parse(savedConfig) as TLegacyConfig; const parsedConfig = migrateLegacyConfig( - JSON.parse(savedConfig) as TLegacyConfig, + rawConfig, ); // check if the config has expired if ( // eslint-disable-next-line `@typescript-eslint/no-unnecessary-condition` -- need to check if expiresAt is set parsedConfig.workspace.expiresAt && new Date(parsedConfig.workspace.expiresAt) <= new Date() ) { return err(new Error("Config in local storage has expired")); } + if (rawConfig.environmentId || rawConfig.environment) { + await AsyncStorage.setItem( + RN_ASYNC_STORAGE_KEY, + JSON.stringify(parsedConfig), + ); + } + return ok(parsedConfig); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-native/src/lib/common/config.ts` around lines 98 - 115, When loadFromStorage() parses savedConfig and calls migrateLegacyConfig(parsed), persist the migrated result back to AsyncStorage under RN_ASYNC_STORAGE_KEY so legacy entries are updated immediately; specifically, after migrateLegacyConfig returns the parsedConfig (inside loadFromStorage in config.ts) call AsyncStorage.setItem(RN_ASYNC_STORAGE_KEY, JSON.stringify(parsedConfig)) (handle/set errors appropriately) so the workspace shape (environmentId/environment) is written without waiting for a later update().packages/react-native/src/types/config.ts (1)
1-6:⚠️ Potential issue | 🟡 MinorUse
@/imports and let Biome organize this block.The new
./workspaceimport keeps this file on relativesrc/references, and CI is already failingassist/source/organizeImports.🧹 Proposed fix
import { z } from "zod"; +import type { TActionClass } from "@/types/action-class"; import type { TResponseUpdate } from "@/types/response"; import type { TFileUploadParams } from "@/types/storage"; -import type { TActionClass } from "./action-class"; -import type { TWorkspace, TWorkspaceStyling } from "./workspace"; -import type { TSurvey } from "./survey"; +import type { TSurvey } from "@/types/survey"; +import type { TWorkspace, TWorkspaceStyling } from "@/types/workspace";As per coding guidelines,
packages/react-native/src/**/*.{ts,tsx}: Use the@/alias when referencingsrc/from withinpackages/react-native.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-native/src/types/config.ts` around lines 1 - 6, The import block uses relative paths for TActionClass, TWorkspace, TWorkspaceStyling, and TSurvey which prevents Biome from organizing imports; change the relative imports "./action-class", "./workspace", and "./survey" to use the src alias "@/types/action-class", "@/types/workspace", and "@/types/survey" so the file uses the "@/..." convention and CI's assist/source/organizeImports will pass (symbols: TActionClass, TWorkspace, TWorkspaceStyling, TSurvey).packages/react-native/src/lib/common/setup.ts (1)
366-387:⚠️ Potential issue | 🟡 Minor
tearDowncan crash ifworkspaceis undefined on the stored config.
const { workspace } = appConfig.get();thenfilterSurveys(workspace, ...)—filterSurveysdestructuresworkspace.data, so iftearDown()is ever called when the config is in an error/partial state without a populatedworkspace(e.g., afterhandleErrorOnFirstSetupwhich writes a config with onlystatus), this will throwTypeError: Cannot read properties of undefined. Consider guarding:Proposed guard
- const { workspace } = appConfig.get(); - - const filteredSurveys = filterSurveys( - workspace, - DEFAULT_USER_STATE_NO_USER_ID, - ); + const currentConfig = appConfig.get(); + const filteredSurveys = currentConfig.workspace + ? filterSurveys(currentConfig.workspace, DEFAULT_USER_STATE_NO_USER_ID) + : [];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-native/src/lib/common/setup.ts` around lines 366 - 387, tearDown can throw when appConfig.get() returns a config without workspace because filterSurveys expects workspace.data; update tearDown to guard the workspace before calling filterSurveys (in function tearDown, replace const { workspace } = appConfig.get(); and the subsequent call with a safe extraction or conditional), e.g. obtain workspace via const { workspace } = appConfig.get() ?? {} or check if workspace and workspace.data exist and only call filterSurveys when present, otherwise set filteredSurveys to an appropriate empty/default value (used with DEFAULT_USER_STATE_NO_USER_ID) before calling appConfig.update and then call removeAllEventListeners as before.
🤖 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-native/src/components/formbricks.tsx`:
- Around line 9-14: The FormbricksProps interface currently makes both
workspaceId and environmentId optional which allows invalid usage like
<Formbricks appUrl="..." />; change the type so at least one identifier is
required by replacing the interface with a union type that enforces "either
workspaceId or environmentId" (e.g., a base { appUrl: string } combined with a
union of { workspaceId: string; environmentId?: never } | { environmentId:
string; workspaceId?: never }) while preserving the deprecated JSDoc on
environmentId; update any references to FormbricksProps accordingly so the
compiler rejects missing identifiers at compile time.
In `@packages/react-native/src/lib/common/setup.ts`:
- Around line 50-84: The fabricated error URL in syncWorkspaceStateIfExpired
(function) hardcodes /api/v1/client/${workspaceId}/environment and can drift
from the real endpoint used by fetchWorkspaceState /
ApiClient.getWorkspaceState; fix this by centralizing URL construction: modify
fetchWorkspaceState (or ApiClient.getWorkspaceState) to include/return the
request URL (or expose a helper that builds the workspace URL) as part of its
error/result, and then consume that returned URL in syncWorkspaceStateIfExpired
when creating the NetworkError response instead of recomputing the path locally.
- Around line 283-295: The code builds effectiveId with a nullish-coalescing
operator so an explicit empty-string workspaceId prevents falling back to
environmentId; change the resolution to treat empty/whitespace workspaceId as
missing (e.g., normalize with .trim() and then use ||) so effectiveId becomes
configInput.workspaceId?.trim() || configInput.environmentId; also update the
deprecation check to use the same normalized test (e.g., if
(configInput.environmentId && !configInput.workspaceId?.trim()) ) and keep the
existing handleMissingField("workspaceId") behavior.
In `@packages/react-native/src/lib/common/tests/config.test.ts`:
- Around line 58-65: Add a new focused Vitest that simulates a legacy persisted
payload containing { environmentId, environment } and verifies loadFromStorage()
migrates it to { workspaceId, workspace }: prepare a stored object (based on
mockConfig) with environmentId and environment (including
environment.data.settings), call loadFromStorage(), assert the returned config
includes workspaceId and workspace with workspace.data.settings preserved,
assert legacy fields environmentId/environment are removed, and assert
AsyncStorage was rewritten (mocked AsyncStorage.setItem called with the new
key/value). Reference loadFromStorage(), mockConfig, and the mocked AsyncStorage
used in this test file when implementing the test.
In `@packages/react-native/src/lib/common/tests/setup.test.ts`:
- Around line 223-249: The test only asserts the deprecation log and leaves the
setup() result unchecked while fetchWorkspaceState is not mocked, which lets
failures slip through; update the test to either mock fetchWorkspaceState (the
function used during sync) to return a successful workspace state so the
positive sync path is exercised, or assert the setup() return value (ok/error)
after calling setup() to ensure the call succeeded; reference the existing mocks
getInstanceConfigMock, mockConfig and mockLogger.debug and ensure the chosen fix
makes setup(...) complete successfully before validating the deprecation log.
In `@packages/react-native/src/lib/common/utils.ts`:
- Around line 1-8: The imports at the top of the file are not sorted per Biome's
organize-imports rule (symbols like TUserState, TWorkspaceState,
TWorkspaceStateSettings, Result, TWorkspaceStyling, TSurvey); run the organizer
to fix ordering (e.g., pnpm biome check --write or run the Biome
organize-imports action) so the import specifiers and source groups are sorted
consistently across the PR; apply the same automated fix wherever CI reports the
same error.
In `@packages/react-native/src/lib/user/update.ts`:
- Around line 27-28: The error URLs in update.ts are using /api/v1/.../user
while ApiClient.createOrUpdateUser() actually calls /api/v2/.../user; update the
constructed url variable(s) and any hardcoded error/report strings (e.g., the
url constant near the top and the similar strings in the block around lines
71-79) to use the /api/v2/client/${workspaceId}/user path so reported endpoints
match the actual request made by ApiClient.createOrUpdateUser().
In `@packages/react-native/src/lib/workspace/state.ts`:
- Around line 138-143: The clearWorkspaceStateExpiryCheckListener function uses
a truthy check on workspaceSyncIntervalId which can skip clearing when the id is
0; change the condition to an explicit null check (e.g., workspaceSyncIntervalId
!== null) so clearInterval is always called for valid numeric IDs and then set
workspaceSyncIntervalId = null; ensure you update the guard in
clearWorkspaceStateExpiryCheckListener to use this explicit comparison
referencing workspaceSyncIntervalId.
- Around line 117-128: The error-path update to appConfig (inside the catch) can
produce a workspace object missing required fields (e.g., data), causing
subsequent calls like filterSurveys or teardown to throw; update the catch logic
in state.ts to merge a sane default workspace shape instead of blindly spreading
existingConfig.workspace: when calling appConfig.update, read const
existingConfig = appConfig.get(), compute workspace = { ...defaultWorkspace,
...(existingConfig.workspace || {}) , expiresAt: new Date(Date.now() + 1000 * 60
* 30) } (where defaultWorkspace provides required keys such as data), and pass
that workspace into appConfig.update so functions like filterSurveys and
handlers such as handleErrorOnFirstSetup always see a valid workspace shape.
- Around line 86-102: The intervalHandler function currently calls
appConfig.get() multiple times (for expiresAt, personState, appUrl,
workspaceId); cache the result once at the top (e.g., const config =
appConfig.get()) and replace uses of appConfig.get() with config to avoid
repeated reads/races, then use const expiresAt = config.workspace.expiresAt and
const personState = config.user and pass config.appUrl and config.workspaceId to
fetchWorkspaceState; keep the existing null/expiry checks (and the eslint
comment if needed) and ensure behavior and logging (logger.debug) remain
unchanged.
- Around line 41-60: The code mutates the network response in-place via the
widened cast on rawData (TWorkspaceState) to copy workspace/project into
settings; instead, create and return a new normalized object without mutating
response.data: inspect response.data.workspace or response.data.project, compute
const normalizedData = { ...response.data, settings: workspace ?? project ??
response.data.settings } (or build a new top-level const normalized = {
...response, data: normalizedData } typed as TWorkspaceState), and use that
normalized object for downstream logic instead of assigning to
rawData.data.settings or deleting properties; update references that used
rawData to use the new normalized object and keep TWorkspaceState types intact.
In `@packages/react-native/src/types/survey.ts`:
- Around line 60-67: The formatter mismatch is caused by the multi-line extends
clause on the SurveyContainerProps interface; collapse the extends clause back
to a single line (e.g., "export interface SurveyContainerProps extends
Omit<SurveyBaseProps, \"onFileUpload\"> {") or run the project formatter (pnpm
biome check --write) to apply Biome's preferred formatting so the CI Biome check
passes; update the block that defines SurveyContainerProps and the Omit
referencing SurveyBaseProps and "onFileUpload".
---
Outside diff comments:
In `@packages/react-native/src/lib/common/config.ts`:
- Around line 98-115: When loadFromStorage() parses savedConfig and calls
migrateLegacyConfig(parsed), persist the migrated result back to AsyncStorage
under RN_ASYNC_STORAGE_KEY so legacy entries are updated immediately;
specifically, after migrateLegacyConfig returns the parsedConfig (inside
loadFromStorage in config.ts) call AsyncStorage.setItem(RN_ASYNC_STORAGE_KEY,
JSON.stringify(parsedConfig)) (handle/set errors appropriately) so the workspace
shape (environmentId/environment) is written without waiting for a later
update().
In `@packages/react-native/src/lib/common/setup.ts`:
- Around line 366-387: tearDown can throw when appConfig.get() returns a config
without workspace because filterSurveys expects workspace.data; update tearDown
to guard the workspace before calling filterSurveys (in function tearDown,
replace const { workspace } = appConfig.get(); and the subsequent call with a
safe extraction or conditional), e.g. obtain workspace via const { workspace } =
appConfig.get() ?? {} or check if workspace and workspace.data exist and only
call filterSurveys when present, otherwise set filteredSurveys to an appropriate
empty/default value (used with DEFAULT_USER_STATE_NO_USER_ID) before calling
appConfig.update and then call removeAllEventListeners as before.
In `@packages/react-native/src/lib/common/tests/__mocks__/config.mock.ts`:
- Around line 87-98: The mock settings in the config mock use the old property
darkOverlay which no longer matches TWorkspaceStateSettings (now expects
overlay); update the settings object in
packages/react-native/src/lib/common/tests/__mocks__/config.mock.ts to replace
darkOverlay with overlay (preserving the boolean value) and ensure the overall
shape matches TWorkspaceStateSettings so you can remove or narrow the unsafe "as
unknown as TConfig" cast; check the settings object and the nested styling block
and adjust types to align with TWorkspaceStateSettings and TConfig so the test
mock reflects production types.
In `@packages/react-native/src/types/config.ts`:
- Around line 1-6: The import block uses relative paths for TActionClass,
TWorkspace, TWorkspaceStyling, and TSurvey which prevents Biome from organizing
imports; change the relative imports "./action-class", "./workspace", and
"./survey" to use the src alias "@/types/action-class", "@/types/workspace", and
"@/types/survey" so the file uses the "@/..." convention and CI's
assist/source/organizeImports will pass (symbols: TActionClass, TWorkspace,
TWorkspaceStyling, TSurvey).
🪄 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: 3bf144dd-ee84-40bb-b498-004779f0361f
📒 Files selected for processing (23)
packages/react-native/src/components/formbricks.tsxpackages/react-native/src/components/survey-web-view.tsxpackages/react-native/src/lib/common/api.tspackages/react-native/src/lib/common/config.tspackages/react-native/src/lib/common/event-listeners.tspackages/react-native/src/lib/common/setup.tspackages/react-native/src/lib/common/tests/__mocks__/config.mock.tspackages/react-native/src/lib/common/tests/api.test.tspackages/react-native/src/lib/common/tests/config.test.tspackages/react-native/src/lib/common/tests/event-listeners.test.tspackages/react-native/src/lib/common/tests/setup.test.tspackages/react-native/src/lib/common/tests/utils.test.tspackages/react-native/src/lib/common/utils.tspackages/react-native/src/lib/environment/state.tspackages/react-native/src/lib/survey/action.tspackages/react-native/src/lib/survey/tests/action.test.tspackages/react-native/src/lib/user/tests/update.test.tspackages/react-native/src/lib/user/update.tspackages/react-native/src/lib/workspace/state.tspackages/react-native/src/lib/workspace/tests/state.test.tspackages/react-native/src/types/config.tspackages/react-native/src/types/survey.tspackages/react-native/src/types/workspace.ts
💤 Files with no reviewable changes (1)
- packages/react-native/src/lib/environment/state.ts
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
React-native equivalent of formbricks/formbricks#7683
Adds support for the sdk to work with
workspaceIdinstead ofenvironmentId. For backwards compatibility,environmentIdis still supported and the local user storage is migrated to follow the new structure on sdk's first run