feat: use strapi feature flags instead of posthog (JUM-844)#2833
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (16)
💤 Files with no reviewable changes (4)
✅ Files skipped from review due to trivial changes (8)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughRoot layout now boots feature-flag evaluation by rendering a new client component. Feature-flag fetching is reworked into hooks and API DTOs; legacy PostHog helper and a monolithic jumper tracking hook were removed and replaced by a tracking utility with direct callers wired to the new flow. ChangesFeature flags, tracking wiring, and API types
Sequence Diagram(s)sequenceDiagram
participant Root as RootLayout
participant FFBoot as FeatureFlagsBootstrap
participant Hook as useFeatureFlags
participant API as Backend (/v1/feature-flags)
participant Tracker as trackJumperEvent/Transaction
participant Jumper as Jumper Backend (/users/events, /wallets/transactions)
Root->>FFBoot: render FeatureFlagsBootstrap
FFBoot->>Hook: call useFeatureFlags()
Hook->>API: GET /v1/feature-flags?distinctId=...
API-->>Hook: FeatureFlagResponseDto[] (cached)
Hook-->>FFBoot: flags available
User->>Tracker: trackEvent / trackTransaction (includes abTestVariants)
Tracker->>Jumper: POST tracking payload
Jumper-->>Tracker: response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
✅ All snapshot tests passed |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/hooks/useABTest.ts (1)
13-16: Remove or deprecate the unusedaddressparameter.
addressis still part of the public hook signature but no longer influences flag resolution, which can confuse callers.♻️ Proposed cleanup
export const useABTest = ({ feature, }: { feature: AbTestName; - address?: string; }): UseABTestProps => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useABTest.ts` around lines 13 - 16, The exported hook signature useABTest currently accepts an unused address parameter (typed in UseABTestProps) which should be removed or marked deprecated to avoid confusion; update the function declaration and the UseABTestProps type to drop the address field (or add a deprecation comment/JSdoc if compatibility required), remove/address any internal references to address in useABTest and ensure all call sites that pass address are updated to stop passing it or to use the new API; also update exported types (AbTestName remains) and run/typecheck to catch remaining usages.src/types/jumper-backend.ts (1)
583-585: TightenabTestVariantsDTO shape for safer payload contracts.Using
objecthere loses useful type guarantees for tracking payload construction.♻️ Proposed type tightening
abtests?: object; - abTestVariants?: object; + abTestVariants?: Record<string, string | boolean>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/types/jumper-backend.ts` around lines 583 - 585, The DTO uses loose type object for abTestVariants (and abtests) which loses compile-time guarantees; replace abTestVariants?: object with a tightened shape such as a named interface or mapped type (e.g., AbTestVariantsDTO or Record<string, string|number|boolean|unknown>) that models variant keys and values, add the corresponding type definition (e.g., interface AbTestVariantsDTO { [testKey: string]: string } or more precise fields as needed), and update the property signature (abTestVariants?: AbTestVariantsDTO) so consumers and serializers have a safer contract; do the same for abtests if it should be tightened.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/hooks/useFeatureFlags.ts`:
- Around line 25-26: The hook useFeatureFlags is building a URL to fetch feature
flags using `${backendUrl}/feature-flags?...` which hits the wrong endpoint;
update the URL construction (the `url` constant) to point to the versioned API
path `/v1/feature-flags` (e.g.
`${backendUrl}/v1/feature-flags?distinctId=${encodeURIComponent(distinctId)}`)
so the fetch call (the `fetch(url)` invocation) hits the correct backend route;
ensure no other callers in this file still reference the non-versioned path.
In `@src/types/jumper-backend.ts`:
- Around line 1014-1019: FeatureFlagResponseDto currently types variant as
object which is too broad; change the variant type to a scalar union (e.g.,
string | number | boolean) so callers no longer need unsafe casts and the
contract matches current A/B usage; update the OpenAPI source/schema for
FeatureFlagResponseDto.variant so regenerated types persist, and run/adjust any
code that assumes object to use the new scalar type (identify usages by
referencing FeatureFlagResponseDto and variant).
---
Nitpick comments:
In `@src/hooks/useABTest.ts`:
- Around line 13-16: The exported hook signature useABTest currently accepts an
unused address parameter (typed in UseABTestProps) which should be removed or
marked deprecated to avoid confusion; update the function declaration and the
UseABTestProps type to drop the address field (or add a deprecation
comment/JSdoc if compatibility required), remove/address any internal references
to address in useABTest and ensure all call sites that pass address are updated
to stop passing it or to use the new API; also update exported types (AbTestName
remains) and run/typecheck to catch remaining usages.
In `@src/types/jumper-backend.ts`:
- Around line 583-585: The DTO uses loose type object for abTestVariants (and
abtests) which loses compile-time guarantees; replace abTestVariants?: object
with a tightened shape such as a named interface or mapped type (e.g.,
AbTestVariantsDTO or Record<string, string|number|boolean|unknown>) that models
variant keys and values, add the corresponding type definition (e.g., interface
AbTestVariantsDTO { [testKey: string]: string } or more precise fields as
needed), and update the property signature (abTestVariants?: AbTestVariantsDTO)
so consumers and serializers have a safer contract; do the same for abtests if
it should be tightened.
🪄 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: CHILL
Plan: Pro
Run ID: 9c8a7ecc-eda5-405b-b8b4-aee936af769c
📒 Files selected for processing (9)
src/app/[lng]/layout.tsxsrc/app/lib/getPosthogFeatureFlag.tssrc/components/FeatureFlagsBootstrap/FeatureFlagsBootstrap.tsxsrc/components/Widgets/Widgets.tsxsrc/hooks/useABTest.tssrc/hooks/useFeatureFlags.tssrc/hooks/useJumperTracking.tssrc/hooks/userTracking/useUserTracking.tssrc/types/jumper-backend.ts
💤 Files with no reviewable changes (2)
- src/components/Widgets/Widgets.tsx
- src/app/lib/getPosthogFeatureFlag.ts
Playwright test resultsDetails
Flaky testschromium › mainMenu.spec.ts › Main Menu flows › Should be able to open mission page and then open the mission (Qase ID: 9) Skipped testschromium › themeManipulation.spec.ts › Switch between dark and light theme and check the background color › Partner theme should appear in theme menu and apply background color (Qase ID: 49) |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/utils/tracking/jumperTracking.ts (1)
84-116: Treat tracking failures as best-effort, not error-reporting signals.These POSTs are expected to fail occasionally during navigation, offline states, or ad-blocking. Sending every failure to Sentry will create noisy telemetry and can obscure real regressions.
Consider
keepalive/sendBeaconfor reliability and limiting Sentry to unexpected failures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/tracking/jumperTracking.ts` around lines 84 - 116, The track function treats every fetch failure as an error and reports to Sentry; change it to be best-effort: use navigator.sendBeacon for POSTs when available (falling back to fetch with { keepalive: true }) from track (used by trackJumperEvent / trackJumperTransaction), and avoid calling captureException for expected transient failures (offline, aborted, ad-blockers). Specifically, modify track to: 1) try navigator.sendBeacon(`${backendUrl}${path}`, JSON.stringify(data)) and return when successful; 2) otherwise use fetch(url, { method: 'POST', headers, body, keepalive: true }) and only call captureException when the failure is clearly non-transient (e.g., response.ok === false with a real status code or an error that is not a TypeError/network-only failure); and 3) short-circuit and do nothing (no Sentry) when navigator.onLine === false or the thrown error is a typical network/abort error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/hooks/useFeatureFlags.ts`:
- Around line 8-12: useFeatureFlagsDistinctId accesses account.address directly
which can be undefined during initial render; change the access to use optional
chaining (account?.address) so the function returns account?.address || fp.
Update the reference in the useFeatureFlagsDistinctId function where
useAccount() and useFpStore(...) are used to guard against missing account data.
---
Nitpick comments:
In `@src/utils/tracking/jumperTracking.ts`:
- Around line 84-116: The track function treats every fetch failure as an error
and reports to Sentry; change it to be best-effort: use navigator.sendBeacon for
POSTs when available (falling back to fetch with { keepalive: true }) from track
(used by trackJumperEvent / trackJumperTransaction), and avoid calling
captureException for expected transient failures (offline, aborted,
ad-blockers). Specifically, modify track to: 1) try
navigator.sendBeacon(`${backendUrl}${path}`, JSON.stringify(data)) and return
when successful; 2) otherwise use fetch(url, { method: 'POST', headers, body,
keepalive: true }) and only call captureException when the failure is clearly
non-transient (e.g., response.ok === false with a real status code or an error
that is not a TypeError/network-only failure); and 3) short-circuit and do
nothing (no Sentry) when navigator.onLine === false or the thrown error is a
typical network/abort error.
🪄 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: CHILL
Plan: Pro
Run ID: a0be2cea-8634-42ba-ad2f-4631fb2349ff
📒 Files selected for processing (11)
src/const/abi/jumperApiUrls.tssrc/const/urls.tssrc/hooks/useFeatureFlags.tssrc/hooks/useJumperTracking.tssrc/hooks/userTracking/useEarnTracking.tssrc/hooks/userTracking/useMissionTracking.tssrc/hooks/userTracking/useUserTracking.tssrc/providers/WidgetTrackingProvider.tsxsrc/types/jumper-backend.tssrc/types/userTracking.tssrc/utils/tracking/jumperTracking.ts
💤 Files with no reviewable changes (2)
- src/const/abi/jumperApiUrls.ts
- src/hooks/useJumperTracking.ts
✅ Files skipped from review due to trivial changes (3)
- src/providers/WidgetTrackingProvider.tsx
- src/hooks/userTracking/useMissionTracking.ts
- src/types/userTracking.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/hooks/userTracking/useUserTracking.ts
- src/types/jumper-backend.ts
5e95421 to
057c3c9
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/hooks/userTracking/useUserTracking.ts`:
- Around line 219-230: The call to trackJumperTransaction in useUserTracking.ts
lacks error handling and can produce unhandled rejections; wrap the entire block
that checks disableTrackingTool?.includes(EventTrackingTool.JumperTracking) and
calls trackJumperTransaction in a try/catch, mirroring how
trackEvent/trackJumperEvent is handled, and on catch log the error (e.g., using
the same logger used elsewhere) without rethrowing so failures are swallowed
safely; ensure you still await trackJumperTransaction and preserve building
payload via buildTransactionPayload, getFeatureFlagVariants and the
abTestVariants merge.
- Around line 79-120: buildTransactionPayload currently calls Number(...) on
potentially undefined fields which yields NaN; update numeric conversions to
provide safe fallbacks so analytics don't get corrupted — for each numeric
property in buildTransactionPayload (fromAmount, fromAmountUSD, gasCostUSD,
toAmount, toAmountMin, toAmountUSD) replace
Number(data[TrackingEventParameter.X]) with a safe conversion that defaults to 0
when the source is undefined (e.g., use Number(data[... ] ?? 0) or an equivalent
check) and keep all other fields unchanged.
🪄 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: CHILL
Plan: Pro
Run ID: d6c5a2a0-65ea-4665-a242-54aee85689c9
📒 Files selected for processing (16)
src/app/[lng]/layout.tsxsrc/app/lib/getPosthogFeatureFlag.tssrc/components/FeatureFlagsBootstrap/FeatureFlagsBootstrap.tsxsrc/components/Widgets/Widgets.tsxsrc/const/abi/jumperApiUrls.tssrc/const/urls.tssrc/hooks/useABTest.tssrc/hooks/useFeatureFlags.tssrc/hooks/useJumperTracking.tssrc/hooks/userTracking/useEarnTracking.tssrc/hooks/userTracking/useMissionTracking.tssrc/hooks/userTracking/useUserTracking.tssrc/providers/WidgetTrackingProvider.tsxsrc/types/jumper-backend.tssrc/types/userTracking.tssrc/utils/tracking/jumperTracking.ts
💤 Files with no reviewable changes (4)
- src/const/abi/jumperApiUrls.ts
- src/hooks/useJumperTracking.ts
- src/components/Widgets/Widgets.tsx
- src/app/lib/getPosthogFeatureFlag.ts
✅ Files skipped from review due to trivial changes (7)
- src/components/FeatureFlagsBootstrap/FeatureFlagsBootstrap.tsx
- src/providers/WidgetTrackingProvider.tsx
- src/types/userTracking.ts
- src/const/urls.ts
- src/hooks/userTracking/useMissionTracking.ts
- src/hooks/useABTest.ts
- src/utils/tracking/jumperTracking.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/hooks/userTracking/useEarnTracking.ts
- src/hooks/useFeatureFlags.ts
- src/types/jumper-backend.ts
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/hooks/userTracking/useUserTracking.ts (1)
216-237:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the resolved transaction action for every tracking sink.
dataActiononly feeds Jumper right now. Ifdata[TrackingEventParameter.Action]differs from theactionprop, GA/Addressable and Jumper will record the same transaction under different event names, which breaks cross-tool attribution.Suggested fix
}: TrackTransactionProps) => { + const dataAction = data[TrackingEventParameter.Action] ?? action ?? ''; if (!disableTrackingTool?.includes(EventTrackingTool.GA)) { - googleEvent({ action, category, data }); + googleEvent({ action: dataAction, category, data }); } if (!disableTrackingTool?.includes(EventTrackingTool.JumperTracking)) { - const dataAction = data[TrackingEventParameter.Action] ?? action ?? ''; const flagVariants = getFeatureFlagVariants(dataAction); await trackJumperTransaction({ ...sessionContext, ...buildTransactionPayload(data), action: dataAction, @@ if (enableAddressable) { addressableEvent({ - action, + action: dataAction, label: 'transaction', data: data || {}, isConversion: true, }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/userTracking/useUserTracking.ts` around lines 216 - 237, Compute a single resolved action (e.g., const resolvedAction = data[TrackingEventParameter.Action] ?? action ?? '') at the top of the transaction block and use that resolvedAction for every tracking sink (googleEvent, trackJumperTransaction's action field, and addressableEvent's action) instead of passing the original action or data[TrackingEventParameter.Action] separately; update references in useUserTracking.ts (googleEvent, trackJumperTransaction payload, and addressableEvent) and keep other payload fields identical (including abTestVariants/flagVariants logic) so all tools record the same event name for cross-tool attribution.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/hooks/userTracking/useUserTracking.ts`:
- Around line 216-237: Compute a single resolved action (e.g., const
resolvedAction = data[TrackingEventParameter.Action] ?? action ?? '') at the top
of the transaction block and use that resolvedAction for every tracking sink
(googleEvent, trackJumperTransaction's action field, and addressableEvent's
action) instead of passing the original action or
data[TrackingEventParameter.Action] separately; update references in
useUserTracking.ts (googleEvent, trackJumperTransaction payload, and
addressableEvent) and keep other payload fields identical (including
abTestVariants/flagVariants logic) so all tools record the same event name for
cross-tool attribution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ec371124-5d43-44cf-b2d5-7bdebbd03c24
📒 Files selected for processing (1)
src/hooks/userTracking/useUserTracking.ts
4cc1d02 to
44f8894
Compare
Which Jira task belongs to this PR?
Contributes to JUM-844
Sister PRs
https://github.com/jumperexchange/strapi-cms/pull/106
https://github.com/jumperexchange/jumper-backend/pull/867
Why did I implement it this way?
Checklist before requesting a review
Summary by CodeRabbit
New Features
Improvements
Changes