Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis PR introduces comprehensive A/B testing (Experiments) functionality across the full stack. The backend adds a new NestJS Experiment module with entities, DTOs, service, and controller supporting experiment lifecycle management and Bayesian statistics calculation. Feature flags integrate with experiments to manage variant assignment. The frontend provides views for creating, managing, and analyzing experiments with results visualization. Supporting database migrations and localization are included. Changes
Sequence DiagramsequenceDiagram
participant User as User/Browser
participant Controller as ExperimentController
participant Service as ExperimentService
participant FeatureFlagSvc as FeatureFlagService
participant DB as Database
participant Analytics as AnalyticsService
participant Bayesian as Bayesian Module
User->>Controller: Start Experiment
Controller->>Service: Validate & fetch experiment
Service->>DB: Get experiment with relations
DB-->>Service: Experiment data
Controller->>FeatureFlagSvc: Create or link feature flag
FeatureFlagSvc->>DB: Save feature flag
DB-->>FeatureFlagSvc: Feature flag created
Controller->>Service: Update experiment status
Service->>DB: Set status=RUNNING, startedAt
DB-->>Service: Updated
Controller-->>User: Started (200)
User->>Controller: Get Experiment Results
Controller->>Analytics: Query exposures & conversions
Analytics-->>Controller: Raw exposure/conversion data
Controller->>Bayesian: calculateBayesianProbabilities(variants, data)
Bayesian-->>Controller: Win probabilities map
Controller->>Controller: generateExperimentChart()
Controller-->>User: Results with probabilities & chart (200)
User->>Controller: Complete Experiment
Controller->>Service: Fetch experiment
Service->>DB: Get experiment
DB-->>Service: Experiment
Controller->>FeatureFlagSvc: Disable feature flag
FeatureFlagSvc->>DB: Update flag enabled=false
DB-->>FeatureFlagSvc: Updated
Controller->>Service: Update status=COMPLETED, endedAt
Service->>DB: Save
DB-->>Service: Updated
Controller-->>User: Completed (200)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring careful attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
web/app/pages/Project/Alerts/View/ProjectAlertsView.tsx (1)
308-315: Deleting an alert doesn’t update the list/total/page (stale UI)
onDeletecallsdeleteAlertApi+ toast, but the UI stays unchanged until a manual reload / paging. This is user-visible and can break pagination totals.Minimal fix (optimistic local update + optional reload to keep totals correct):
const onDelete = async (id: string) => { try { await deleteAlertApi(id) toast.success(t('alertsSettings.alertDeleted')) + setAlerts((prev) => prev.filter((a) => a.id !== id)) + setTotal((prev) => Math.max(0, prev - 1)) + // Optional: if you prefer server-truth (and to handle page underflow), reload: + // loadAlerts(DEFAULT_ALERTS_TAKE, (page - 1) * DEFAULT_ALERTS_TAKE) } catch (reason: any) { toast.error(reason?.response?.data?.message || reason?.message || 'Something went wrong') } }Also applies to: 396-446
web/app/pages/Project/Alerts/Settings/ProjectAlertsSettings.tsx (1)
40-62:onCloseis optional but “Cancel” assumes it exists
If a caller forgetsonClose, Cancel becomes a silent no-op. Either makeonCloserequired, or provide a safe default (e.g., navigate tobackLinkor a known alerts route).interface ProjectAlertsSettingsProps { alertId?: string | null projectId: string isSettings?: boolean - onClose?: () => void + onClose: () => void onSave?: () => void backLink?: string }Also applies to: 543-579
web/app/pages/Project/Profiles/View/ProfilesView.tsx (2)
63-136: IncrementprofilesRequestIdRef(stale responses can overwrite newer state).
const requestId = profilesRequestIdRef.currentis never advanced, so the “only apply latest response” guard is ineffective.const loadProfiles = async (forcedSkip?: number, override?: boolean) => { if (profilesLoading) { return } - const requestId = profilesRequestIdRef.current + const requestId = ++profilesRequestIdRef.current setProfilesLoading(true)
138-184: Profile/sessions refresh can be skipped + stale profile loads can win.
loadProfileSessionsData(..., override=true)is called aftergetProfile, but if a sessions request is already running, the earlyif (profileSessionsLoading) returndrops the refresh.- Rapidly switching
profileIdcan setactiveProfilefrom an older in-flight request (no request-id check).Suggestion: add per-request ids for
loadProfileandloadProfileSessionsData(and allow override to bypass the “loading” short-circuit), e.g.+ const profileRequestIdRef = useRef(0) + const profileSessionsRequestIdRef = useRef(0) const loadProfile = async (profileId: string) => { + const requestId = ++profileRequestIdRef.current const hasExistingProfileData = !!activeProfile && activeProfile.profileId === profileId setProfileLoading(true) ... const data = await getProfile(...) - if (isMountedRef.current) { + if (isMountedRef.current && requestId === profileRequestIdRef.current) { setActiveProfile(data) loadProfileSessionsData(profileId, 0, true) } } const loadProfileSessionsData = async (profileId: string, forcedSkip?: number, override?: boolean) => { - if (profileSessionsLoading) { + if (profileSessionsLoading && !override) { return } + const requestId = ++profileSessionsRequestIdRef.current setProfileSessionsLoading(true) ... - if (isMountedRef.current) { + if (isMountedRef.current && requestId === profileSessionsRequestIdRef.current) { ... }Also applies to: 186-258
🟡 Minor comments (14)
web/app/pages/Project/Experiments/View/ExperimentSettingsModal.tsx-672-674 (1)
672-674: Multiple hardcoded English strings in Advanced settings section.Several UI strings are hardcoded: "Advanced settings", "Feature flag source", "Create new", "Link existing", "Default", "Custom event", "Exclude", "First only", and tooltip contents.
These should use translation keys for i18n support.
Also applies to: 686-690, 704-705, 716-717, 770-771, 782-783, 808-812, 829-830, 841-842
web/app/pages/Project/Experiments/View/ExperimentResults.tsx-449-449 (1)
449-449: Hardcoded "Value" column header should use translation.- <TableHeader>Value</TableHeader> + <TableHeader>{t('experiments.value')}</TableHeader>web/app/pages/Project/Experiments/View/ExperimentResults.tsx-574-578 (1)
574-578: Hardcoded "Significant" text should use translation.The "Significant" label is hardcoded in English while other labels use
t()for i18n.{variant.probabilityOfBeingBest >= 95 ? ( <span className='rounded-full bg-green-100 px-1.5 py-0.5 text-[10px] font-medium text-green-700 dark:bg-green-900/50 dark:text-green-300'> - Significant + {t('experiments.significant')} </span> ) : null}web/app/pages/Project/Experiments/View/ExperimentResults.tsx-391-407 (1)
391-407: Hardcoded "Total" text should use translation.The "Total" row label is hardcoded in English.
<tr className='bg-gray-50 dark:bg-slate-800'> <TableCell> <Text weight='semibold' size='sm'> - Total + {t('common.total')} </Text> </TableCell>web/app/pages/Project/Experiments/View/ExperimentResults.tsx-181-202 (1)
181-202: Tooltip HTML template could be vulnerable to XSS if variant names contain malicious content.The
el.nameis interpolated directly into HTML without sanitization. If a variant name contained HTML/script content, it could be rendered. While this data likely comes from your own backend, consider sanitizing or escaping the name.${_map(items, (el: { id: string; name: string; value: number }) => { + const safeName = el.name.replace(/</g, '<').replace(/>/g, '>') return ` <li class='flex justify-between'> <div class='flex justify-items-start'> <div class='w-3 h-3 rounded-xs mt-1.5 mr-2' style=background-color:${color(el.id)}></div> - <span>${el.name}</span> + <span>${safeName}</span> </div> <span class='pl-4 font-medium'>${el.value}%</span> </li> ` }).join('')}</ul>`web/app/pages/Project/Experiments/View/ExperimentSettingsModal.tsx-599-606 (1)
599-606: Tooltip text should use translation key.Hardcoded English tooltip text for goal selection.
<Tooltip - text='Select a goal to measure which variant performs better. You can start the experiment without a goal and add one later.' + text={t('experiments.goalHint')} className='flex items-center' />web/app/pages/Project/Experiments/View/ExperimentSettingsModal.tsx-459-461 (1)
459-461: Tooltip text should use translation key.Hardcoded English tooltip text for variants explanation.
<Tooltip - text='Define the variants users will see. The control is your baseline (usually the current version). Other variants are what you want to test.' + text={t('experiments.variantsHint')} className='flex items-center' />web/app/pages/Project/View/components/TrafficHeaderActions.tsx-166-172 (1)
166-172: Hardcoded export type in analytics tracking.The analytics tracking always reports
type: 'csv'regardless of the actual export type selected. If multiple export formats are supported (or planned), consider passing the actual type from theExportTypeitem.Apply this diff if export types should be tracked accurately:
interface ExportType { label: string + type: string onClick: () => void }onSelect={(item) => { trackCustom('DASHBOARD_EXPORT', { - type: 'csv', + type: item.type, }) item.onClick() }}Committable suggestion skipped: line range outside the PR's diff.
web/app/pages/Project/FeatureFlags/View/FeatureFlagsView.tsx-790-865 (1)
790-865: Empty state logic doesn't account for search with no results.When
filterQueryhas a value but the search returns no results (flagsis empty), the onboarding CTA ("Add your first flag") is shown instead of the "no flags match filter" message. This could confuse users who have flags but are searching for something that doesn't exist.Consider adjusting the condition to differentiate between "no flags exist" and "no flags match the filter":
- {_isEmpty(flags) ? ( + {_isEmpty(flags) && !filterQuery ? ( <div className='mt-5 rounded-xl bg-gray-700 p-5'> ... </div> + ) : _isEmpty(flags) && filterQuery ? ( + <p className='py-8 text-center text-sm text-gray-500 dark:text-gray-400'> + {t('featureFlags.noFlagsMatchFilter')} + </p> ) : (web/app/pages/Project/View/components/DashboardHeader.tsx-21-39 (1)
21-39: Prevent rendering two back buttons when bothbackLinkandonBackare provided.
Prefer a clear precedence (e.g.,onBackwins) or warn in prop typing/docs.- {backLink ? <BackButton to={backLink} /> : null} - {onBack ? <BackButton onClick={onBack} /> : null} + {onBack ? <BackButton onClick={onBack} /> : backLink ? <BackButton to={backLink} /> : null}Also applies to: 123-130
web/app/pages/Project/Performance/View/PerformanceView.tsx-634-690 (1)
634-690: Avoid remounting Panels viakey={activeTabs...}; also localize “Loading map…”.Using
key={activeTabs.location|device|page}forces remounts on tab switches (state loss + extra work). Prefer a stable key (e.g.,type). Also the Suspense fallback string is hardcoded (Line 661-669).- <Panel - key={activeTabs.location} + <Panel + key={type} icon={panelIconMapping.cc} id={activeTabs.location} ... - <Panel - key={activeTabs.device} + <Panel + key={type} icon={panelIconMapping.os} id={activeTabs.device} ... - <Panel - key={activeTabs.page} + <Panel + key={type} icon={panelIconMapping.pg} id={activeTabs.page} ... - <span className='text-sm text-neutral-600 dark:text-neutral-300'> - Loading map... - </span> + <span className='text-sm text-neutral-600 dark:text-neutral-300'> + {t('common.loading')} + </span>Also applies to: 699-723, 735-775, 661-669
web/app/pages/Project/Experiments/View/ExperimentsView.tsx-219-225 (1)
219-225: Localize hardcoded tooltips (“Pause this experiment…”, “Completed experiments…”).backend/apps/cloud/src/experiment/experiment.controller.ts-281-335 (1)
281-335: Non-atomic experiment creation with linked feature flag.If the experiment is created successfully (line 317) but the feature flag update fails (line 321-323), the experiment will exist without proper flag linkage. Consider wrapping in a transaction.
// Consider using a transaction for atomicity: await this.experimentService.transaction(async (manager) => { const newExperiment = await manager.save(experiment) if (existingFeatureFlag) { await this.featureFlagService.update(existingFeatureFlag.id, { experimentId: newExperiment.id, }) } return newExperiment })backend/apps/cloud/src/experiment/experiment.controller.ts-984-986 (1)
984-986: Edge case: empty variants array could cause error.If
variantResultsis empty (e.g., no variants), thereducecall without an initial value will throw. While this shouldn't happen given prior validation, defensive coding would be safer.- const highestProbVariant = variantResults.reduce((a, b) => - a.probabilityOfBeingBest > b.probabilityOfBeingBest ? a : b, - ) + const highestProbVariant = variantResults.length > 0 + ? variantResults.reduce((a, b) => + a.probabilityOfBeingBest > b.probabilityOfBeingBest ? a : b, + ) + : null - const hasWinner = highestProbVariant.probabilityOfBeingBest >= 95 + const hasWinner = highestProbVariant + ? highestProbVariant.probabilityOfBeingBest >= 95 + : false
🧹 Nitpick comments (32)
web/app/ui/Button.tsx (1)
8-26: Add a variant-precedence rule (or dev-time guard) forghostvs other variants.Right now callers can pass
ghostalongsideprimary/secondary/danger/white/semiDanger/noBorder, which can create conflicting Tailwind utilities (e.g.,border+border-none,bg-*collisions). Consider enforcing exclusivity (union type) or at least documenting/guarding precedence (e.g.,ghostwins, orghostis ignored whenprimaryetc. is set).Also applies to: 28-49
web/app/pages/Project/Alerts/View/ProjectAlertsView.tsx (2)
220-227:backLinkbecomes"?"when there are no remaining params
WhenpureSearchParamsis empty,backLink={?${pureSearchParams}}yields"?", which is a weird navigation target (and may break consumers that expect''/undefined). Consider only prefixing?when non-empty.- backLink={`?${pureSearchParams}`} + backLink={pureSearchParams ? `?${pureSearchParams}` : ''}Also applies to: 391-393
278-307: Clear conflicting URL params when switching modes (edit vs create)
handleNewAlert()setsnewAlert=truebut doesn’t clearalertId;openAlert()setsalertIdbut doesn’t clearnewAlert. It likely “works” due toactiveAlertId || isCreatingAlert, but leaves stale params in the URL.const handleNewAlert = () => { if (isLimitReached) { setIsPaidFeatureOpened(true) return } const newSearchParams = new URLSearchParams(searchParams.toString()) + newSearchParams.delete('alertId') newSearchParams.set('newAlert', 'true') setSearchParams(newSearchParams) } const openAlert = (alertId: string) => { const newSearchParams = new URLSearchParams(searchParams.toString()) + newSearchParams.delete('newAlert') newSearchParams.set('alertId', alertId) setSearchParams(newSearchParams) }web/app/pages/Project/Alerts/Settings/ProjectAlertsSettings.tsx (2)
82-118: Load sequencing based onalertIdlooks OK; consider clearing stalealerton mode switch
When switching from edit → create (alertIdbecomes null), you setisLoading(false)but keepalertas-is until overwritten. ConsidersetAlert(null)(and maybe resetting form) to avoid any transient stale state.if (alertId) { loadAlert(alertId) } else { + setAlert(null) setIsLoading(false) }
269-283: Delete flow correctly gates onalertId; consider also closing after delete
AfterdeleteAlert, youonSave?.()but don’t necessarily close the settings view. If the parent doesn’t close on save (today it does), you’ll be left on a now-invalid edit screen. ConsideronClose?.()(or document that parent must close)..then(() => { toast.success(t('alertsSettings.alertDeleted')) onSave?.() + onClose?.() })Also applies to: 581-591
web/app/pages/Project/View/components/ProjectSidebar.tsx (3)
509-518: Consider adding focus management for accessibility.The mobile sidebar overlay is well-implemented with backdrop click-to-close and ESC key handling. However, for improved accessibility:
- When the sidebar opens, focus should move to the sidebar (or the close button).
- Consider trapping focus within the sidebar while it's open.
This ensures keyboard-only users can navigate the mobile sidebar effectively.
Additionally, verify that
animate-slide-in-leftis defined in your Tailwind config or CSS:#!/bin/bash # Verify the animate-slide-in-left class is defined rg -n "slide-in-left" --type=css --type=js --type=ts
453-453: Simplify boolean expression.The ternary is redundant for a boolean expression.
- isCollapsed={isCollapsed && !isMobileOpen ? true : false} + isCollapsed={isCollapsed && !isMobileOpen}
255-271: Minor: Consider preserving original overflow value.The current implementation resets
overflowto an empty string, which could unintentionally override a previously set value if another component modifies body overflow.useEffect(() => { const handleEscape = (e: KeyboardEvent) => { if (e.key === 'Escape' && isMobileOpen) { onMobileClose?.() } } + let originalOverflow: string | undefined if (isMobileOpen) { document.addEventListener('keydown', handleEscape) + originalOverflow = document.body.style.overflow document.body.style.overflow = 'hidden' } return () => { document.removeEventListener('keydown', handleEscape) - document.body.style.overflow = '' + if (originalOverflow !== undefined) { + document.body.style.overflow = originalOverflow + } } }, [isMobileOpen, onMobileClose])backend/migrations/clickhouse/2025_12_11_experiments.js (1)
15-15: Consider including profileId in the ORDER BY clause.The current ordering
(pid, experimentId, created)supports project and experiment-level queries but may not optimize queries filtering byprofileId(e.g., "which variant did user X see?").If queries frequently filter by profileId, consider updating the ORDER BY:
- ORDER BY (pid, experimentId, created) + ORDER BY (pid, experimentId, profileId, created)This change would improve performance for user-centric experiment queries while maintaining efficiency for project-level aggregations.
web/app/pages/Project/Experiments/View/ExperimentResults.tsx (2)
161-166: Type cast in tick format function is safe but could be cleaner.The
@ts-expect-errorandx as unknown as Datecasts work but are awkward. The d3timeFormatexpects a Date, and billboard.js passes the Date object here. Consider typing the parameter directly asDatesince billboard.js provides Date objects for timeseries axes.format: - // @ts-expect-error - timeFormat === TimeFormat['24-hour'] - ? (x: string) => d3.timeFormat(tbsFormatMapper24h[timeBucket])(x as unknown as Date) - : (x: string) => d3.timeFormat(tbsFormatMapper[timeBucket])(x as unknown as Date), + timeFormat === TimeFormat['24-hour'] + ? (x: Date) => d3.timeFormat(tbsFormatMapper24h[timeBucket])(x) + : (x: Date) => d3.timeFormat(tbsFormatMapper[timeBucket])(x),
658-702: Data loading effect handles errors gracefully but doesn't preserve previous data on refresh failure.The current implementation clears
erroron each attempt but doesn't roll back state if a refresh fails after initial load. The comment on line 665 suggests intent to keep existing data, which is good. However, ifgetExperimentorgetExperimentResultsfails, the component will show the error state even if previous data existed.Consider only showing the error UI when there's no existing data:
The current behavior at lines 718-735 already handles this correctly by checking
!experiment || !resultsbefore showing error. The implementation is correct as-is.web/app/pages/Project/Experiments/View/ExperimentSettingsModal.tsx (5)
141-150: Missing cleanup for loading state if modal closes during load.If the modal is closed while
loadExperimentis in progress, the finally block will still setisLoading(false)on a potentially unmounted component. Consider adding a mounted ref or using AbortController.+ const isMountedRef = useRef(true) + + useEffect(() => { + isMountedRef.current = true + return () => { + isMountedRef.current = false + } + }, []) + const loadExperiment = async () => { if (!experimentId) return setIsLoading(true) try { const experiment = await getExperiment(experimentId) + if (!isMountedRef.current) return setName(experiment.name) // ... rest of setters } catch (error) { + if (!isMountedRef.current) return console.error('Failed to load experiment:', error) toast.error(t('experiments.loadError')) onClose() } finally { - setIsLoading(false) + if (isMountedRef.current) setIsLoading(false) } }
188-232: Validation logic is comprehensive but could report multiple errors.Currently, if multiple variant-related errors exist, only the last one is stored in
newErrors.variants. Consider accumulating errors or prioritizing the most critical one.The current approach is acceptable for UX as it guides users through fixing one issue at a time.
285-302: Variant naming uses character codes that can produce unexpected results.When
variantIndexis 1 (second variant being added),String.fromCharCode(64 + 1)= 'A' andString.fromCharCode(96 + 1)= 'a'. But when the array already has 2 variants,variantIndexis 2, producing 'B' and 'b'. This is correct but could be confusing if variants are removed and re-added.Consider using a counter that only increments, or basing the letter on the actual count of non-control variants to avoid duplicate letters after removals. Current behavior is acceptable.
480-482: Using index as key for variant list items.While this works when items are only added/removed from the end, it can cause issues if items are reordered or removed from the middle. Consider using a stable identifier.
{_map(variants, (variant, index) => ( <div - key={index} + key={variant.key || index} className={cx(Since variant keys should be unique after validation, this provides a more stable key.
634-635: Hidden text on small screens may affect usability.The "Increase"/"Decrease" labels are hidden on small screens (
hidden sm:inline), leaving only icons. Consider keeping at least abbreviated text or ensuring icons are self-explanatory with aria-labels.Also applies to: 647-648
backend/apps/cloud/src/experiment/experiment.module.ts (1)
15-23: Multiple forwardRef usages - verify necessity.Five of six module imports use
forwardRef, which may indicate tight coupling. While the circular dependency withFeatureFlagModuleis expected (experiments integrate with feature flags), verify whetherUserModule,ProjectModule,AnalyticsModule, andGoalModuletruly needforwardRefor if they can be direct imports.If these modules don't have circular dependencies with
ExperimentModule, remove the unnecessaryforwardRefwrappers to simplify the module configuration and improve clarity.web/app/pages/Project/Errors/View/ErrorsView.tsx (1)
770-801: Simplify redundant optional chaining.After checking
activeError &&, usingactiveError?.details?.statusis redundant. The?.onactiveErrorisn't needed since it's already verified as truthy.- const resolveButton = - allowedToManage && activeError && activeError?.details?.status !== 'resolved' ? ( + const resolveButton = + allowedToManage && activeError && activeError.details?.status !== 'resolved' ? ( <button ... </button> - ) : allowedToManage && activeError && activeError?.details?.status === 'resolved' ? ( + ) : allowedToManage && activeError && activeError.details?.status === 'resolved' ? (backend/apps/cloud/src/feature-flag/entity/feature-flag.entity.ts (1)
60-63: Consider indexingexperimentId(and/or modeling a relation) if it’s queried frequently.
If you’ll look up feature flags by experiment, add an index to avoid table scans.import { Column, CreateDateColumn, Entity, JoinColumn, ManyToOne, + Index, PrimaryGeneratedColumn, Unique, } from 'typeorm' ... + @Index(['experimentId']) @Entity() @Unique(['project', 'key']) export class FeatureFlag {web/app/pages/Project/View/components/CaptchaView.tsx (1)
270-334: Avoid@ts-expect-errorfor logo maps; type the keys instead.
These suppressions tend to spread and hide real issues.Example approach:
- Type the maps as
Record<string, string>(orRecord<BrowserName, string>if you have a union), then index without suppression.- Or cast the lookup key once:
const key = entryName as keyof typeof BROWSER_LOGO_MAP.backend/apps/cloud/src/feature-flag/feature-flag.controller.ts (2)
321-334: Optional: dedupe experiment IDs before querying.
398-430: Consider skipping experiment exposure insert when flag-evaluations insert fails.Right now exposures can be recorded even if
feature_flag_evaluationsinsert errored (Line 398-430), which may complicate downstream analytics.backend/apps/cloud/src/experiment/dto/experiment.dto.ts (2)
23-49: CoercerolloutPercentageto number for validation reliability.export class ExperimentVariantDto { @ApiProperty() @IsNumber() @Min(0) @Max(100) + @Type(() => Number) rolloutPercentage: number
117-122: Enforce basic variant constraints (min 2 variants) and validate cross-variant invariants in service.At DTO level, consider
@ArrayMinSize(2)(and possibly@ArrayMaxSize(...)). In service-level validation, ensure: uniquekey, exactly oneisControl, and rollout percentages sum to 100.import { ... + ArrayMinSize, } from 'class-validator' export class CreateExperimentDto { ... @ApiProperty({ type: [ExperimentVariantDto] }) @IsArray() + @ArrayMinSize(2) @ValidateNested({ each: true }) @Type(() => ExperimentVariantDto) variants: ExperimentVariantDto[] }Also applies to: 187-193
backend/apps/cloud/src/experiment/experiment.service.ts (1)
75-85: Consider returning typed results from update/delete operations.The
updateanddeletemethods returnPromise<any>. Consider usingPromise<UpdateResult>andPromise<DeleteResult>from TypeORM for better type safety.- async update(id: string, experimentData: Partial<Experiment>): Promise<any> { + async update(id: string, experimentData: Partial<Experiment>): Promise<UpdateResult> { return this.experimentRepository.update(id, experimentData) } - async delete(id: string): Promise<any> { + async delete(id: string): Promise<DeleteResult> { return this.experimentRepository.delete(id) }Note: You'll need to import
UpdateResultandDeleteResultfromtypeorm.web/app/pages/Project/View/ViewProject.tsx (1)
739-758: Minor: Unused variables in onMainChartZoom.
fromFormattedandtoFormattedare declared withletbut never reassigned. Consider usingconst.- let fromFormatted = from.toISOString().split('T')[0] + 'T00:00:00.000Z' - let toFormatted = to.toISOString().split('T')[0] + 'T23:59:59.999Z' + const fromFormatted = from.toISOString().split('T')[0] + 'T00:00:00.000Z' + const toFormatted = to.toISOString().split('T')[0] + 'T23:59:59.999Z'backend/apps/cloud/src/feature-flag/evaluation.ts (1)
208-215: Consider logging when fallback is triggered.The fallback to the last variant (line 218) handles cases where percentages don't sum to 100. This could indicate a configuration error. Consider adding debug logging or metrics when this fallback is hit.
// Fallback to last variant (shouldn't happen if percentages sum to 100) + // Note: If this code path is hit, it may indicate variant percentages don't sum to 100 return variants[variants.length - 1].keyweb/app/api/index.ts (2)
1570-1586:CreateExperimentinterface is not exported.Unlike other similar types in this file (e.g.,
CreateAlert,CreateGoal,CreateFeatureFlag), this interface is not exported. If consumers need to construct experiment payloads outside this module, they won't have access to this type.Consider exporting for consistency:
-interface CreateExperiment { +export interface CreateExperiment {
1590-1609:getProjectExperimentslacks search parameter support.Unlike
getProjectFeatureFlags(lines 1382-1410), which accepts and handles asearchparameter,getProjectExperimentsdoesn't support searching/filtering experiments by name. This may be intentional if the backend doesn't support it yet.backend/apps/cloud/src/experiment/experiment.controller.ts (3)
568-651:startExperimentcreates feature flag with truncated ID.Line 604-605: The generated flag key truncates the experiment ID to 20 characters. While unlikely to cause collisions, this could theoretically result in duplicate keys if two experiment IDs share the same first 20 characters (after replacing dashes).
Consider using a hash or the full ID:
- `experiment_${experiment.id.replace(/-/g, '_').substring(0, 20)}` + `exp_${experiment.id.replace(/-/g, '_')}`
636-640: RedundantfeatureFlagassignment in update.The
featureFlagis already set on the entity viacreate(line 625) or was fetched from the database. Passing it again in the update payload may trigger unnecessary ORM operations.await this.experimentService.update(id, { status: ExperimentStatus.RUNNING, startedAt: new Date(), - featureFlag, })
981-987: Winner determination uses hardcoded 95% threshold.The
hasWinnercheck at line 987 uses a hardcoded 95% threshold. This is a reasonable default for statistical significance, but consider making it configurable per experiment if different confidence levels are needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
web/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (62)
backend/apps/cloud/src/app.module.ts(2 hunks)backend/apps/cloud/src/experiment/bayesian.ts(1 hunks)backend/apps/cloud/src/experiment/dto/experiment.dto.ts(1 hunks)backend/apps/cloud/src/experiment/entity/experiment-variant.entity.ts(1 hunks)backend/apps/cloud/src/experiment/entity/experiment.entity.ts(1 hunks)backend/apps/cloud/src/experiment/experiment.controller.ts(1 hunks)backend/apps/cloud/src/experiment/experiment.module.ts(1 hunks)backend/apps/cloud/src/experiment/experiment.service.ts(1 hunks)backend/apps/cloud/src/feature-flag/dto/feature-flag.dto.ts(1 hunks)backend/apps/cloud/src/feature-flag/entity/feature-flag.entity.ts(2 hunks)backend/apps/cloud/src/feature-flag/evaluation.ts(5 hunks)backend/apps/cloud/src/feature-flag/feature-flag.controller.ts(4 hunks)backend/apps/cloud/src/feature-flag/feature-flag.module.ts(2 hunks)backend/apps/cloud/src/feature-flag/feature-flag.service.ts(1 hunks)backend/apps/cloud/src/project/entity/project.entity.ts(2 hunks)backend/apps/community/src/feature-flag/dto/feature-flag.dto.ts(1 hunks)backend/apps/community/src/feature-flag/entity/feature-flag.entity.ts(1 hunks)backend/apps/community/src/feature-flag/evaluation.ts(3 hunks)backend/migrations/clickhouse/2025_12_11_experiments.js(1 hunks)backend/migrations/mysql/2025_12_11_experiments.sql(1 hunks)web/.env.example(0 hunks)web/app/api/index.ts(5 hunks)web/app/components/DangerouslySetInnerHTML.tsx(0 hunks)web/app/hooks/useAnnotations.ts(1 hunks)web/app/lib/constants/index.ts(3 hunks)web/app/pages/Project/Alerts/Settings/ProjectAlertsSettings.tsx(11 hunks)web/app/pages/Project/Alerts/Settings/index.tsx(0 hunks)web/app/pages/Project/Alerts/View/ProjectAlertsView.tsx(7 hunks)web/app/pages/Project/Errors/View/ErrorsView.tsx(3 hunks)web/app/pages/Project/Experiments/View/ExperimentResults.tsx(1 hunks)web/app/pages/Project/Experiments/View/ExperimentSettingsModal.tsx(1 hunks)web/app/pages/Project/Experiments/View/ExperimentsView.tsx(1 hunks)web/app/pages/Project/Experiments/View/index.tsx(1 hunks)web/app/pages/Project/FeatureFlags/View/FeatureFlagsView.tsx(2 hunks)web/app/pages/Project/Funnels/View/FunnelsView.tsx(4 hunks)web/app/pages/Project/Goals/View/GoalsView.tsx(2 hunks)web/app/pages/Project/Performance/View/PerformanceView.tsx(4 hunks)web/app/pages/Project/Profiles/View/ProfilesView.tsx(7 hunks)web/app/pages/Project/Sessions/View/SessionsView.tsx(3 hunks)web/app/pages/Project/Traffic/View/TrafficView.tsx(7 hunks)web/app/pages/Project/View/Panels.tsx(2 hunks)web/app/pages/Project/View/ViewProject.helpers.tsx(5 hunks)web/app/pages/Project/View/ViewProject.tsx(25 hunks)web/app/pages/Project/View/components/BackButton.tsx(1 hunks)web/app/pages/Project/View/components/CaptchaView.tsx(6 hunks)web/app/pages/Project/View/components/DashboardHeader.tsx(2 hunks)web/app/pages/Project/View/components/Errors.tsx(0 hunks)web/app/pages/Project/View/components/ProjectSidebar.tsx(19 hunks)web/app/pages/Project/View/components/SessionDetailView.tsx(1 hunks)web/app/pages/Project/View/components/SessionDetails.tsx(0 hunks)web/app/pages/Project/View/components/TrafficHeaderActions.tsx(3 hunks)web/app/routes/projects.$pid.alerts.create.tsx(0 hunks)web/app/routes/projects.$pid.alerts.settings.$id.tsx(0 hunks)web/app/styles/tailwind.css(2 hunks)web/app/ui/Button.tsx(3 hunks)web/app/ui/FilterValueInput.tsx(3 hunks)web/app/ui/Switch.tsx(2 hunks)web/app/utils/generic.ts(0 hunks)web/app/utils/routes.ts(0 hunks)web/package.json(0 hunks)web/public/locales/en.json(3 hunks)web/vite.config.ts(2 hunks)
💤 Files with no reviewable changes (10)
- web/app/utils/routes.ts
- web/package.json
- web/app/utils/generic.ts
- web/app/pages/Project/Alerts/Settings/index.tsx
- web/app/pages/Project/View/components/SessionDetails.tsx
- web/app/components/DangerouslySetInnerHTML.tsx
- web/app/routes/projects.$pid.alerts.settings.$id.tsx
- web/app/routes/projects.$pid.alerts.create.tsx
- web/app/pages/Project/View/components/Errors.tsx
- web/.env.example
🧰 Additional context used
🧬 Code graph analysis (21)
backend/apps/cloud/src/experiment/experiment.module.ts (3)
backend/apps/cloud/src/app.module.ts (1)
Module(95-109)backend/apps/cloud/src/feature-flag/feature-flag.module.ts (1)
Module(13-26)web/app/api/index.ts (2)
Experiment(1518-1539)ExperimentVariant(1509-1516)
backend/apps/cloud/src/experiment/entity/experiment-variant.entity.ts (3)
backend/apps/cloud/src/experiment/entity/experiment.entity.ts (1)
Entity(42-138)backend/apps/cloud/src/project/entity/project.entity.ts (1)
Entity(31-178)web/app/api/index.ts (2)
ExperimentVariant(1509-1516)Experiment(1518-1539)
backend/apps/cloud/src/project/entity/project.entity.ts (1)
web/app/api/index.ts (1)
Experiment(1518-1539)
web/app/pages/Project/FeatureFlags/View/FeatureFlagsView.tsx (1)
web/app/api/index.ts (1)
DEFAULT_FEATURE_FLAGS_TAKE(1380-1380)
backend/migrations/clickhouse/2025_12_11_experiments.js (1)
backend/migrations/clickhouse/setup.js (2)
dbName(107-107)queriesRunner(76-105)
web/app/ui/FilterValueInput.tsx (1)
web/app/pages/Project/View/ViewProject.helpers.tsx (1)
deviceIconMapping(1677-1685)
web/app/pages/Project/View/components/BackButton.tsx (1)
web/app/utils/generic.ts (1)
cn(150-152)
web/app/pages/Project/Goals/View/GoalsView.tsx (1)
web/app/api/index.ts (1)
DEFAULT_GOALS_TAKE(1258-1258)
web/app/pages/Project/Experiments/View/ExperimentResults.tsx (3)
web/app/ui/Text.tsx (1)
Text(51-75)web/app/api/index.ts (6)
ExperimentChartData(1552-1555)ExperimentResults(1557-1568)Experiment(1518-1539)Goal(1232-1242)getExperiment(1611-1617)getExperimentResults(1667-1682)web/app/lib/constants/index.ts (5)
TimeFormat(245-248)tbsFormatMapper24h(237-243)tbsFormatMapper(213-219)tbsFormatMapperTooltip24h(229-235)tbsFormatMapperTooltip(221-227)
web/app/pages/Project/Experiments/View/ExperimentSettingsModal.tsx (1)
web/app/api/index.ts (8)
ExperimentVariant(1509-1516)Goal(1232-1242)ExposureTrigger(1503-1503)MultipleVariantHandling(1505-1505)FeatureFlagMode(1507-1507)getExperiment(1611-1617)updateExperiment(1627-1633)createExperiment(1619-1625)
web/app/pages/Project/Alerts/View/ProjectAlertsView.tsx (4)
web/app/providers/CurrentProjectProvider.tsx (1)
useCurrentProject(244-252)web/app/providers/AuthProvider.tsx (1)
useAuth(130-138)web/app/utils/server.ts (1)
isAuthenticated(46-48)web/app/lib/constants/index.ts (1)
DEFAULT_ALERTS_TAKE(449-449)
backend/apps/cloud/src/feature-flag/feature-flag.controller.ts (2)
web/app/api/index.ts (1)
ExperimentStatus(1501-1501)backend/apps/cloud/src/feature-flag/evaluation.ts (1)
getExperimentVariant(187-219)
backend/apps/cloud/src/experiment/dto/experiment.dto.ts (1)
web/app/api/index.ts (4)
ExposureTrigger(1503-1503)MultipleVariantHandling(1505-1505)FeatureFlagMode(1507-1507)ExperimentStatus(1501-1501)
web/app/pages/Project/Sessions/View/SessionsView.tsx (1)
web/app/pages/Project/View/components/Sessions.tsx (1)
Sessions(220-238)
web/app/pages/Project/View/components/ProjectSidebar.tsx (1)
web/app/utils/generic.ts (1)
cn(150-152)
backend/apps/cloud/src/feature-flag/evaluation.ts (2)
backend/apps/community/src/feature-flag/evaluation.ts (1)
EvaluatableFeatureFlag(24-30)web/app/api/index.ts (1)
ExperimentVariant(1509-1516)
web/app/pages/Project/Traffic/View/TrafficView.tsx (7)
web/app/pages/Project/View/interfaces/traffic.ts (2)
ProjectView(26-31)ProjectViewCustomEvent(17-24)web/app/pages/Project/View/ViewProject.helpers.tsx (4)
onCSVExportClick(1983-1983)CHART_METRICS_MAPPING(1984-1984)noRegionPeriods(1981-1981)getDeviceRowMapper(1717-1833)web/app/pages/Project/View/utils/filters.tsx (1)
FILTER_CHART_METRICS_MAPPING_FOR_COMPARE(16-16)web/app/pages/Project/View/components/MetricCards.tsx (2)
MetricCards(186-261)MetricCard(66-94)web/app/lib/constants/index.ts (1)
TRAFFIC_PANELS_ORDER(172-172)web/app/lib/models/Entry.ts (2)
CountryEntry(8-11)Entry(1-6)web/app/pages/Project/View/components/ChartContextMenu.tsx (1)
ChartContextMenu(20-138)
web/app/pages/Project/Performance/View/PerformanceView.tsx (3)
web/app/pages/Project/View/components/MetricCards.tsx (1)
PerformanceMetricCards(269-318)web/app/lib/constants/index.ts (1)
PERFORMANCE_PANELS_ORDER(173-173)web/app/pages/Project/View/components/ChartContextMenu.tsx (1)
ChartContextMenu(20-138)
web/app/pages/Project/View/components/CaptchaView.tsx (3)
web/app/providers/CurrentProjectProvider.tsx (1)
useCurrentProject(244-252)web/app/lib/constants/index.ts (3)
BROWSER_LOGO_MAP(858-905)OS_LOGO_MAP(907-933)OS_LOGO_MAP_DARK(935-940)web/app/pages/Project/View/ViewProject.helpers.tsx (2)
panelIconMapping(1979-1979)deviceIconMapping(1677-1685)
web/app/pages/Project/View/components/TrafficHeaderActions.tsx (1)
web/app/utils/analytics.ts (1)
trackCustom(173-182)
web/app/pages/Project/Funnels/View/FunnelsView.tsx (2)
web/app/pages/Project/View/ViewProject.tsx (1)
useViewProjectContext(219-222)web/app/ui/Text.tsx (1)
Text(51-75)
1 participant
Changes
If applicable, please describe what changes were made in this pull request.
Community Edition support
Database migrations
Documentation
Summary by CodeRabbit
Release Notes
New Features
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.