Feat/ab testing#1
Conversation
|
/review |
Changelist by BitoThis pull request implements the following key changes.
|
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #e7f837
Actionable Suggestions - 1
-
packages/features/flags/ab-testing.test.ts - 1
- Incorrect test assertion · Line 353-353
Additional Suggestions - 2
-
packages/features/flags/ab-testing.test.ts - 1
-
Weak test coverage for independence · Line 334-336The assertions only check that values are booleans, but the test comment suggests verifying independence. Consider adding checks for variation across features.
-
-
packages/features/flags/ab-testing.ts - 1
-
Code Duplication · Line 11-84The hashing and rollout logic is duplicated between user and team functions. Refactoring into shared helpers would improve maintainability and reduce the risk of inconsistencies if changes are needed.
-
Review Details
-
Files reviewed - 6 · Commit Range:
c3e2adf..7815576- packages/features/flags/ab-testing.test.ts
- packages/features/flags/ab-testing.ts
- packages/features/flags/features.repository.integration-test.ts
- packages/features/flags/features.repository.ts
- packages/prisma/migrations/20251028145907_add_ab_testing_to_features/migration.sql
- packages/prisma/schema.prisma
-
Files skipped - 1
- packages/features/package.json - Reason: Filter setting
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Default Agent You can customize the agent settings here or contact your Bito workspace admin at dhiren.m@codity.ai.
Documentation & Help
AI Code Review powered by
| const bucketV2 = getUserBucket(userId, feature, "production-v2"); | ||
|
|
||
| // Buckets should be different (not guaranteed but highly likely) | ||
| expect([true, false]).toContain(bucketV1 !== bucketV2); |
There was a problem hiding this comment.
The assertion here always passes since it checks if a boolean is true or false. It should verify that buckets actually differ with different salts, as intended by the comment.
Code suggestion
Check the AI-generated fix before applying
| expect([true, false]).toContain(bucketV1 !== bucketV2); | |
| expect(bucketV1).not.toBe(bucketV2); |
Code Review Run #e7f837
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
This comment was marked as resolved.
This comment was marked as resolved.
1 similar comment
|
@codity review |
|
PR review started! Estimated time: 40-45 minutes. 💡 Learn MoreAsk Codity questions: Trigger a manual review: Generate unit tests: Run security scan again: |
PR SummaryWhat Changed
Key Changes by Area
Files Changed
Potential Impact
Review Focus Areas
|
Workflow DiagramsAutomatically generated sequence diagrams showing the workflows in this PR 1. Service Integration FlowComplex complexity • Components: ab-testing.ts, ab-testing.test.ts, features.repository.ts sequenceDiagram
title A/B Testing Feature Integration
participant User
participant Frontend
participant API
participant Database
participant A/B Testing Service
participant Test Suite
User->>Frontend: Interacts with A/B testing feature
Frontend->>API: Request user bucket for feature
API->>A/B Testing Service: Get user bucket
A/B Testing Service-->>API: Return user bucket
API-->>Frontend: Send user bucket
Frontend->>User: Display feature based on bucket
User->>Frontend: Triggers feature usage
Frontend->>API: Log feature usage
API->>Database: Store feature usage data
Database-->>API: Confirm storage
API-->>Frontend: Acknowledge log
User->>Frontend: Requests feature rollout status
Frontend->>API: Check if user is in rollout
API->>A/B Testing Service: Check rollout status
A/B Testing Service-->>API: Return rollout status
API-->>Frontend: Send rollout status
Frontend->>User: Display rollout status
Note over Frontend, Test Suite: Test suite validates A/B testing logic
Test Suite->>A/B Testing Service: Run tests for user bucket logic
A/B Testing Service-->>Test Suite: Return test results
Test Suite->>API: Validate API responses
API-->>Test Suite: Send API responses
Test Suite->>User: Report test outcomes
Note: Diagrams show detected patterns only. Complex workflows may require manual review. |
![codity-dev[bot]](https://avatars.githubusercontent.com/u/228176587?s=60&v=4){kind=small}
| const bytes = Buffer.from(input, "utf-8"); | ||
|
|
||
| const hash = x86.hash32(bytes); | ||
|
|
There was a problem hiding this comment.
Using Buffer ties this code to a Node.js environment. In browser contexts (e.g., client-side bundles), Buffer is undefined, causing a ReferenceError at runtime. This breaks deterministic bucketing on the client.
Code Suggestion or Comments
export function getUserBucket(userId: number, featureSlug: string, salt: string = ""): number {
// Combine userId, feature, and salt into a single string
const input = `${userId}:${featureSlug}:${salt}`;
// Avoid Node-only Buffer; hash directly from string for universal (SSR/CSR) environments
const hash = x86.hash32(input);
// Convert to 0-99 range for percentage-based bucketing (unsigned to avoid negative values)
return (hash >>> 0) % 100;
}Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert typescript developer with deep knowledge of security, performance, and best practices.
### Context
File: packages/features/flags/ab-testing.ts
Lines: 15-18
Issue Type: functional-critical
Severity: critical
Issue Description:
Using Buffer ties this code to a Node.js environment. In browser contexts (e.g., client-side bundles), Buffer is undefined, causing a ReferenceError at runtime. This breaks deterministic bucketing on the client.
Current Code:
const bytes = Buffer.from(input, "utf-8");
const hash = x86.hash32(bytes);
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow typescript best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| const bytes = Buffer.from(input, "utf-8"); | ||
|
|
||
| // Use MurmurHash3 x86 32-bit variant for fast, uniform distribution | ||
| const hash = x86.hash32(bytes); |
There was a problem hiding this comment.
Using Buffer in a potentially browser-executed module will fail at runtime because Buffer is not available without a polyfill. This breaks team bucketing in client bundles.
Code Suggestion or Comments
export function getTeamBucket(teamId: number, featureSlug: string, salt: string = ""): number {
// Combine teamId, feature, and salt into a single string
// Use "team:" prefix to ensure different distribution from users
const input = `team:${teamId}:${featureSlug}:${salt}`;
// Avoid Node-only Buffer; hash directly from string for universal (SSR/CSR) environments
const hash = x86.hash32(input);
// Convert to 0-99 range for percentage-based bucketing (unsigned to avoid negative values)
return (hash >>> 0) % 100;
}Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert typescript developer with deep knowledge of security, performance, and best practices.
### Context
File: packages/features/flags/ab-testing.ts
Lines: 55-58
Issue Type: functional-critical
Severity: critical
Issue Description:
Using Buffer in a potentially browser-executed module will fail at runtime because Buffer is not available without a polyfill. This breaks team bucketing in client bundles.
Current Code:
// Convert string to bytes (murmurhash3js-revisited requires byte input)
const bytes = Buffer.from(input, "utf-8");
// Use MurmurHash3 x86 32-bit variant for fast, uniform distribution
const hash = x86.hash32(bytes);
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow typescript best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| @@ -0,0 +1,3 @@ | |||
| -- AlterTable | |||
| ALTER TABLE "public"."Feature" ADD COLUMN "rolloutPercentage" INTEGER NOT NULL DEFAULT 100, | |||
There was a problem hiding this comment.
The rolloutPercentage column lacks a CHECK constraint to enforce valid bounds (0-100). Without this, invalid values can be inserted, breaking feature rollout logic and causing undefined behavior.
Code Suggestion or Comments
ALTER TABLE "public"."Feature"
ADD COLUMN "rolloutPercentage" INTEGER NOT NULL DEFAULT 100;
ALTER TABLE "public"."Feature"
ADD CONSTRAINT "Feature_rolloutPercentage_range_check"
CHECK ("rolloutPercentage" BETWEEN 0 AND 100);Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert sql developer with deep knowledge of security, performance, and best practices.
### Context
File: packages/prisma/migrations/20251028145907_add_ab_testing_to_features/migration.sql
Lines: 2-2
Issue Type: robustness-high
Severity: high
Issue Description:
The rolloutPercentage column lacks a CHECK constraint to enforce valid bounds (0-100). Without this, invalid values can be inserted, breaking feature rollout logic and causing undefined behavior.
Current Code:
ALTER TABLE "public"."Feature" ADD COLUMN "rolloutPercentage" INTEGER NOT NULL DEFAULT 100,
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow sql best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| @@ -0,0 +1,3 @@ | |||
| -- AlterTable | |||
| ALTER TABLE "public"."Feature" ADD COLUMN "rolloutPercentage" INTEGER NOT NULL DEFAULT 100, | |||
| ADD COLUMN "salt" TEXT NOT NULL DEFAULT ''; | |||
There was a problem hiding this comment.
The salt column defaults to an empty string, which undermines randomness and can make traffic bucketing predictable across features. A non-empty, pseudo-random default should be used and existing empty values backfilled.
Code Suggestion or Comments
ALTER TABLE "public"."Feature"
ADD COLUMN "salt" TEXT NOT NULL DEFAULT md5(random()::text || clock_timestamp()::text);
-- Backfill any existing rows that may have an empty salt
UPDATE "public"."Feature"
SET "salt" = md5(random()::text || clock_timestamp()::text)
WHERE "salt" = '';Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert sql developer with deep knowledge of security, performance, and best practices.
### Context
File: packages/prisma/migrations/20251028145907_add_ab_testing_to_features/migration.sql
Lines: 3-3
Issue Type: security-high
Severity: high
Issue Description:
The salt column defaults to an empty string, which undermines randomness and can make traffic bucketing predictable across features. A non-empty, pseudo-random default should be used and existing empty values backfilled.
Current Code:
ADD COLUMN "salt" TEXT NOT NULL DEFAULT '';
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow sql best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| // Use the A/B testing utility with the feature's rollout percentage and salt | ||
| return isUserInRollout(userId, slug, feature.rolloutPercentage, feature.salt); | ||
| } catch (err) { | ||
| captureException(err); |
There was a problem hiding this comment.
captureException is used in multiple catch blocks but there is no visible import for it in this file. This will result in a compile/runtime error due to an undefined identifier.
Code Suggestion or Comments
import { captureException } from "@sentry/nextjs";Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert typescript developer with deep knowledge of security, performance, and best practices.
### Context
File: packages/features/flags/features.repository.ts
Lines: 306-306
Issue Type: functional-high
Severity: high
Issue Description:
captureException is used in multiple catch blocks but there is no visible import for it in this file. This will result in a compile/runtime error due to an undefined identifier.
Current Code:
captureException(err);
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow typescript best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| // Rollout percentage for A/B testing (0-100, default 100 = full rollout) | ||
| rolloutPercentage Int @default(100) |
There was a problem hiding this comment.
The rolloutPercentage is documented to be 0-100 but lacks any enforced constraint. Invalid values could break feature rollout logic and lead to unexpected exposure or withholding of features.
Code Suggestion or Comments
// Rollout percentage for A/B testing (0-100, default 100 = full rollout)
rolloutPercentage Int @default(100) @db.SmallInt
-- Migration (SQL) to enforce range at DB level
ALTER TABLE "Feature"
ADD CONSTRAINT "Feature_rolloutPercentage_range"
CHECK ("rolloutPercentage" BETWEEN 0 AND 100);Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert prisma developer with deep knowledge of security, performance, and best practices.
### Context
File: packages/prisma/schema.prisma
Lines: 1646-1647
Issue Type: functional-high
Severity: high
Issue Description:
The rolloutPercentage is documented to be 0-100 but lacks any enforced constraint. Invalid values could break feature rollout logic and lead to unexpected exposure or withholding of features.
Current Code:
// Rollout percentage for A/B testing (0-100, default 100 = full rollout)
rolloutPercentage Int @default(100)
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow prisma best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| it("should demonstrate re-randomization with salt change", () => { | ||
| const userId = 12345; | ||
| const feature = "test-feature"; | ||
| const percentage = 50; | ||
|
|
||
| const inRolloutV1 = isUserInRollout(userId, feature, percentage, "production-v1"); | ||
| const inRolloutV2 = isUserInRollout(userId, feature, percentage, "production-v2"); | ||
|
|
||
| // Same user, same percentage, but different salt can produce different results | ||
| // We can't guarantee they're different for one user, but the function supports it | ||
| const bucketV1 = getUserBucket(userId, feature, "production-v1"); | ||
| const bucketV2 = getUserBucket(userId, feature, "production-v2"); | ||
|
|
||
| // Buckets should be different (not guaranteed but highly likely) | ||
| expect([true, false]).toContain(bucketV1 !== bucketV2); | ||
| }); |
There was a problem hiding this comment.
This assertion is tautological and will always pass regardless of behavior. It does not verify that changing the salt re-randomizes bucket assignment and may mask regressions.
Code Suggestion or Comments
it("should demonstrate re-randomization with salt change", () => {
const feature = "test-feature";
const percentage = 50;
let differCount = 0;
const totalUsers = 1000;
for (let userId = 1; userId <= totalUsers; userId++) {
const inRolloutV1 = isUserInRollout(userId, feature, percentage, "production-v1");
const inRolloutV2 = isUserInRollout(userId, feature, percentage, "production-v2");
if (inRolloutV1 !== inRolloutV2) differCount++;
}
// At least 40% of users should differ between salts for a 50% rollout
expect(differCount).toBeGreaterThan(totalUsers * 0.4);
});Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert typescript developer with deep knowledge of security, performance, and best practices.
### Context
File: packages/features/flags/ab-testing.test.ts
Lines: 339-354
Issue Type: functional-high
Severity: high
Issue Description:
This assertion is tautological and will always pass regardless of behavior. It does not verify that changing the salt re-randomizes bucket assignment and may mask regressions.
Current Code:
it("should demonstrate re-randomization with salt change", () => {
const userId = 12345;
const feature = "test-feature";
const percentage = 50;
const inRolloutV1 = isUserInRollout(userId, feature, percentage, "production-v1");
const inRolloutV2 = isUserInRollout(userId, feature, percentage, "production-v2");
// Same user, same percentage, but different salt can produce different results
// We can't guarantee they're different for one user, but the function supports it
const bucketV1 = getUserBucket(userId, feature, "production-v1");
const bucketV2 = getUserBucket(userId, feature, "production-v2");
// Buckets should be different (not guaranteed but highly likely)
expect([true, false]).toContain(bucketV1 !== bucketV2);
});
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow typescript best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| @@ -0,0 +1,3 @@ | |||
| -- AlterTable | |||
| ALTER TABLE "public"."Feature" ADD COLUMN "rolloutPercentage" INTEGER NOT NULL DEFAULT 100, | |||
There was a problem hiding this comment.
Defaulting rolloutPercentage to 100 rolls features out fully by default for any enabled feature. Safer defaults typically start at 0 to avoid unintended global rollouts.
Code Suggestion or Comments
ALTER TABLE "public"."Feature"
ADD COLUMN "rolloutPercentage" INTEGER NOT NULL DEFAULT 0;Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert sql developer with deep knowledge of security, performance, and best practices.
### Context
File: packages/prisma/migrations/20251028145907_add_ab_testing_to_features/migration.sql
Lines: 2-2
Issue Type: functional-medium
Severity: medium
Issue Description:
Defaulting rolloutPercentage to 100 rolls features out fully by default for any enabled feature. Safer defaults typically start at 0 to avoid unintended global rollouts.
Current Code:
ALTER TABLE "public"."Feature" ADD COLUMN "rolloutPercentage" INTEGER NOT NULL DEFAULT 100,
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow sql best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| "class-variance-authority": "^0.7.1", | ||
| "framer-motion": "^10.12.8", | ||
| "lexical": "^0.9.0", | ||
| "murmurhash3js-revisited": "^3.0.0", |
There was a problem hiding this comment.
Using a caret (^) version range allows automatic minor/patch updates, which can introduce unexpected or malicious changes (supply-chain risk). Pinning to an exact version improves determinism and reduces attack surface.
Code Suggestion or Comments
"murmurhash3js-revisited": "3.0.0",Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert json developer with deep knowledge of security, performance, and best practices.
### Context
File: packages/features/package.json
Lines: 26-26
Issue Type: security-medium
Severity: medium
Issue Description:
Using a caret (^) version range allows automatic minor/patch updates, which can introduce unexpected or malicious changes (supply-chain risk). Pinning to an exact version improves determinism and reduces attack surface.
Current Code:
"murmurhash3js-revisited": "^3.0.0",
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow json best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| // Rollout percentage for A/B testing (0-100, default 100 = full rollout) | ||
| rolloutPercentage Int @default(100) | ||
| // Salt for deterministic user bucketing, allows re-randomization when changed | ||
| salt String @default("") |
There was a problem hiding this comment.
Using an empty string as the default salt can couple user bucketing across multiple flags, making experiments inadvertently correlated. A per-row random default salt reduces cross-flag correlation risk.
Code Suggestion or Comments
salt String @default(uuid())Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert prisma developer with deep knowledge of security, performance, and best practices.
### Context
File: packages/prisma/schema.prisma
Lines: 1649-1649
Issue Type: maintainability-medium
Severity: medium
Issue Description:
Using an empty string as the default salt can couple user bucketing across multiple flags, making experiments inadvertently correlated. A per-row random default salt reduces cross-flag correlation risk.
Current Code:
salt String @default("")
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow prisma best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| // Salt for deterministic user bucketing, allows re-randomization when changed | ||
| salt String @default("") | ||
| lastUsedAt DateTime? | ||
| createdAt DateTime? @default(now()) |
There was a problem hiding this comment.
createdAt is optional despite having a default. Allowing nulls for a creation timestamp can lead to inconsistent records and complicate auditing and ordering by creation time.
Code Suggestion or Comments
createdAt DateTime @default(now())Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert prisma developer with deep knowledge of security, performance, and best practices.
### Context
File: packages/prisma/schema.prisma
Lines: 1651-1651
Issue Type: robustness-medium
Severity: medium
Issue Description:
createdAt is optional despite having a default. Allowing nulls for a creation timestamp can lead to inconsistent records and complicate auditing and ordering by creation time.
Current Code:
createdAt DateTime? @default(now())
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow prisma best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| salt String @default("") | ||
| lastUsedAt DateTime? | ||
| createdAt DateTime? @default(now()) | ||
| updatedAt DateTime? @default(now()) @updatedAt |
There was a problem hiding this comment.
updatedAt is optional and also has @default(now()) with @updatedat. Typically, updatedAt should be non-nullable and rely solely on @updatedat for automatic updates to avoid nulls and redundant defaults.
Code Suggestion or Comments
updatedAt DateTime @updatedAtPrompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert prisma developer with deep knowledge of security, performance, and best practices.
### Context
File: packages/prisma/schema.prisma
Lines: 1652-1652
Issue Type: maintainability-medium
Severity: medium
Issue Description:
updatedAt is optional and also has @default(now()) with @updatedAt. Typically, updatedAt should be non-nullable and rely solely on @updatedAt for automatic updates to avoid nulls and redundant defaults.
Current Code:
updatedAt DateTime? @default(now()) @updatedAt
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow prisma best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| it("should distribute users uniformly across buckets", () => { | ||
| const feature = "test-feature"; | ||
| const salt = "v1"; | ||
| const bucketCounts: number[] = new Array(100).fill(0); | ||
|
|
||
| // Test 10,000 users | ||
| for (let userId = 1; userId <= 10000; userId++) { | ||
| const bucket = getUserBucket(userId, feature, salt); | ||
| bucketCounts[bucket]++; | ||
| } | ||
|
|
||
| // Each bucket should have roughly 100 users (10,000 / 100) | ||
| // Allow for variance - expect each bucket to have between 50 and 150 users | ||
| bucketCounts.forEach((count) => { | ||
| expect(count).toBeGreaterThan(50); | ||
| expect(count).toBeLessThan(150); | ||
| }); | ||
|
|
||
| // Calculate standard deviation to ensure uniform distribution | ||
| const mean = 10000 / 100; // 100 | ||
| const variance = | ||
| bucketCounts.reduce((sum, count) => sum + Math.pow(count - mean, 2), 0) / bucketCounts.length; | ||
| const stdDev = Math.sqrt(variance); | ||
|
|
||
| // Standard deviation should be relatively small for uniform distribution | ||
| expect(stdDev).toBeLessThan(15); | ||
| }); |
There was a problem hiding this comment.
The test iterates over 10,000 users and performs multiple assertions per bucket. This can significantly slow down the test suite without materially improving confidence. Consider reducing the sample size and relaxing thresholds accordingly.
Code Suggestion or Comments
const feature = "test-feature";
const salt = "v1";
const bucketCounts: number[] = new Array(100).fill(0);
// Reduce sample size to speed up tests while maintaining confidence
for (let userId = 1; userId <= 2000; userId++) {
const bucket = getUserBucket(userId, feature, salt);
bucketCounts[bucket]++;
}
// With 2,000 users, expect ~20 per bucket; allow reasonable variance
bucketCounts.forEach((count) => {
expect(count).toBeGreaterThan(10);
expect(count).toBeLessThan(30);
});
const mean = 2000 / 100; // 20
const variance =
bucketCounts.reduce((sum, count) => sum + Math.pow(count - mean, 2), 0) / bucketCounts.length;
const stdDev = Math.sqrt(variance);
expect(stdDev).toBeLessThan(8);Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert typescript developer with deep knowledge of security, performance, and best practices.
### Context
File: packages/features/flags/ab-testing.test.ts
Lines: 82-108
Issue Type: performance-medium
Severity: medium
Issue Description:
The test iterates over 10,000 users and performs multiple assertions per bucket. This can significantly slow down the test suite without materially improving confidence. Consider reducing the sample size and relaxing thresholds accordingly.
Current Code:
const feature = "test-feature";
const salt = "v1";
const bucketCounts: number[] = new Array(100).fill(0);
// Test 10,000 users
for (let userId = 1; userId <= 10000; userId++) {
const bucket = getUserBucket(userId, feature, salt);
bucketCounts[bucket]++;
}
// Each bucket should have roughly 100 users (10,000 / 100)
// Allow for variance - expect each bucket to have between 50 and 150 users
bucketCounts.forEach((count) => {
expect(count).toBeGreaterThan(50);
expect(count).toBeLessThan(150);
});
// Calculate standard deviation to ensure uniform distribution
const mean = 10000 / 100; // 100
const variance =
bucketCounts.reduce((sum, count) => sum + Math.pow(count - mean, 2), 0) / bucketCounts.length;
const stdDev = Math.sqrt(variance);
// Standard deviation should be relatively small for uniform distribution
expect(stdDev).toBeLessThan(15);
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow typescript best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| it("should distribute teams uniformly across buckets", () => { | ||
| const feature = "test-feature"; | ||
| const salt = "v1"; | ||
| const bucketCounts: number[] = new Array(100).fill(0); | ||
|
|
||
| // Test 10,000 teams | ||
| for (let teamId = 1; teamId <= 10000; teamId++) { | ||
| const bucket = getTeamBucket(teamId, feature, salt); | ||
| bucketCounts[bucket]++; | ||
| } | ||
|
|
||
| // Each bucket should have roughly 100 teams (10,000 / 100) | ||
| bucketCounts.forEach((count) => { | ||
| expect(count).toBeGreaterThan(50); | ||
| expect(count).toBeLessThan(150); | ||
| }); | ||
|
|
||
| // Calculate standard deviation | ||
| const mean = 10000 / 100; | ||
| const variance = | ||
| bucketCounts.reduce((sum, count) => sum + Math.pow(count - mean, 2), 0) / bucketCounts.length; | ||
| const stdDev = Math.sqrt(variance); | ||
|
|
||
| expect(stdDev).toBeLessThan(15); | ||
| }); |
There was a problem hiding this comment.
This team distribution test uses 10,000 iterations and strict thresholds, which can make the test suite slow and brittle. Reducing iterations and adjusting expectations keeps the test meaningful while improving performance.
Code Suggestion or Comments
const feature = "test-feature";
const salt = "v1";
const bucketCounts: number[] = new Array(100).fill(0);
// Reduce iterations to speed up CI runs
for (let teamId = 1; teamId <= 2000; teamId++) {
const bucket = getTeamBucket(teamId, feature, salt);
bucketCounts[bucket]++;
}
// With 2,000 teams, expect ~20 per bucket; allow reasonable variance
bucketCounts.forEach((count) => {
expect(count).toBeGreaterThan(10);
expect(count).toBeLessThan(30);
});
const mean = 2000 / 100;
const variance =
bucketCounts.reduce((sum, count) => sum + Math.pow(count - mean, 2), 0) / bucketCounts.length;
const stdDev = Math.sqrt(variance);
expect(stdDev).toBeLessThan(8);Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert typescript developer with deep knowledge of security, performance, and best practices.
### Context
File: packages/features/flags/ab-testing.test.ts
Lines: 402-426
Issue Type: performance-medium
Severity: medium
Issue Description:
This team distribution test uses 10,000 iterations and strict thresholds, which can make the test suite slow and brittle. Reducing iterations and adjusting expectations keeps the test meaningful while improving performance.
Current Code:
const feature = "test-feature";
const salt = "v1";
const bucketCounts: number[] = new Array(100).fill(0);
// Test 10,000 teams
for (let teamId = 1; teamId <= 10000; teamId++) {
const bucket = getTeamBucket(teamId, feature, salt);
bucketCounts[bucket]++;
}
// Each bucket should have roughly 100 teams (10,000 / 100)
bucketCounts.forEach((count) => {
expect(count).toBeGreaterThan(50);
expect(count).toBeLessThan(150);
});
// Calculate standard deviation
const mean = 10000 / 100;
const variance =
bucketCounts.reduce((sum, count) => sum + Math.pow(count - mean, 2), 0) / bucketCounts.length;
const stdDev = Math.sqrt(variance);
expect(stdDev).toBeLessThan(15);
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow typescript best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
Nitpicks (Low Priority)Found 3 low-priority suggestions for code improvement Click to expand nitpicks
|
|
PR review is completed! Running some final security checks... |
🔐 Security Scan Summary
Consider addressing security findings before merging Scan completed in 215.2s 📋 Top 20 Critical Vulnerabilities (34 total found)1. SEC-061 (CWE-862) 🟠 HIGH 📁
💡 Fix: Enforce authorization in this method (e.g., require admin role or pass an authenticated actor and verify permissions) before allowing updates. 2. SEC-008 (CWE-327) 🟡 MEDIUM 📁
💡 Fix: If gating sensitive features or permissions, replace with a keyed HMAC (e.g., HMAC-SHA256) using a server-side secret pepper. Keep Murmur only for non-security-critical experimentation. 3. SEC-008 (CWE-327) 🟡 MEDIUM 📁
💡 Fix: If used to gate sensitive team features, use a keyed HMAC (e.g., HMAC-SHA256) with a server-side secret. Maintain Murmur only for purely experimental rollouts. 4. SEC-014 (CWE-330) 🟡 MEDIUM 📁
💡 Fix: Require a non-empty environment-specific secret salt (e.g., from configuration) and avoid defaulting to an empty string. 5. SEC-014 (CWE-330) 🟡 MEDIUM 📁
💡 Fix: Disallow empty salt; require an environment-specific, secret salt for production rollouts. 6. SEC-014 (CWE-330) 🟡 MEDIUM 📁
💡 Fix: Require a non-empty secret salt value from configuration for rollout tier calculation. 7. SEC-014 (CWE-330) 🟡 MEDIUM 📁
💡 Fix: Enforce non-empty salt sourced from secure configuration; reject empty salt in production. 8. SEC-014 (CWE-330) 🟡 MEDIUM 📁
💡 Fix: Require non-empty, environment-specific salt for deterministic but unpredictable bucketing. 9. SEC-014 (CWE-330) 🟡 MEDIUM 📁
💡 Fix: Do not default to empty salt; require a configured, secret salt for production usage. 10. SEC-021 (CWE-20) 🟡 MEDIUM 📁
💡 Fix: Validate at runtime: ensure userId is a finite integer >= 0, featureSlug is a non-empty, normalized string, and salt meets length/character constraints. 11. SEC-021 (CWE-20) 🟡 MEDIUM 📁
💡 Fix: Validate userId is a finite integer and featureSlug is a non-empty string with a safe character set before computing the bucket. 12. SEC-021 (CWE-20) 🟡 MEDIUM 📁
💡 Fix: Validate inputs similar to getUserBucket: enforce types, ranges, and non-empty strings. 13. SEC-021 (CWE-20) 🟡 MEDIUM 📁
💡 Fix: Ensure teamId is a finite integer >= 0 and featureSlug is a non-empty normalized string before hashing. 14. SEC-021 (CWE-20) 🟡 MEDIUM 📁
💡 Fix: Validate teamId is a finite integer and featureSlug is a non-empty string prior to hashing. 15. SEC-021 (CWE-20) 🟡 MEDIUM 📁
💡 Fix: Add input validation for teamId (finite integer) and featureSlug (non-empty, normalized string). 16. SEC-030 (CWE-400) 🟡 MEDIUM 📁
💡 Fix: Enforce max length for featureSlug and salt (e.g., 100 chars); reject overly long inputs before Buffer.from. 17. SEC-030 (CWE-400) 🟡 MEDIUM 📁
💡 Fix: Limit length of featureSlug and salt; validate teamId; enforce reasonable limits before creating buffers. 18. SEC-021 (CWE-20) 🟡 MEDIUM 📁
💡 Fix: Validate salt length (e.g., <= 128 chars) and allowed charset before persisting. 19. SEC-021 (CWE-20) 🟡 MEDIUM 📁
💡 Fix: Validate slug against an allowlist of known feature slugs or ensure it matches expected pattern before use. 20. SEC-021 (CWE-20) 🟡 MEDIUM 📁
💡 Fix: Validate slug against known features or a strict regex before invoking rollout logic.
🛡️ Security scan powered by Codity.ai |
2 participants
What does this PR do?
Visual Demo (For contributors especially)
A visual demonstration is strongly recommended, for both the original and new change (video / image - any one).
Video Demo (if applicable):
Image Demo (if applicable):
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Checklist
Summary by Bito
This PR implements A/B testing functionality for Cal.com, adding utilities for user and team bucketing, rollout management, and database schema updates to support feature experimentation.
Detailed Changes