Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
|
Warning Rate limit exceeded@cstrnt has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 49 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (9)
WalkthroughThis pull request introduces extensive updates across documentation, database schema, UI components, API endpoints, core libraries, and tests. Documentation now includes new metadata entries and expanded sections on feature flags, user segments, and rule operators. New Prisma tables and models for user segments and flag rule sets are added, with API queries updated accordingly. Various UI components receive accessibility, layout, and styling improvements, while several pages are enhanced with new layouts and features such as the Flag Detail and Segments tab. Core library functions have been updated to include new type parameters and an added Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant UI as Frontend
participant A as Abby Instance
participant V as Validation Module
U->>UI: Submit new user properties
UI->>A: Call updateUserProperties(newProps)
A->>V: Validate newProps
V-->>A: Return validation result
A->>A: Update internal user state and re-evaluate flags
A->>UI: Return updated flag evaluations
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
60dadd8 to
68c1f94
Compare
68c1f94 to
79eef4b
Compare
fix(web): update feature flag component to use link and improve data handling
…gModal components
There was a problem hiding this comment.
Actionable comments posted: 8
🔭 Outside diff range comments (4)
apps/web/src/components/CreateEnvironmentModal.tsx (1)
24-24:⚠️ Potential issueFix inconsistency between name validation and submission.
The validation uses
trimmedNamebut the mutation usesname, which could allow leading/trailing whitespace in environment names.try { await mutateAsync({ - name, + name: trimmedName, projectId, });Also applies to: 39-39, 45-45
apps/web/src/components/CodeSnippet.tsx (2)
94-97:⚠️ Potential issueSanitize HTML content before using dangerouslySetInnerHTML.
Using dangerouslySetInnerHTML without sanitization could expose the application to XSS attacks.
+import DOMPurify from 'dompurify'; <div - dangerouslySetInnerHTML={{ __html: props[currentIntegration].html }} + dangerouslySetInnerHTML={{ + __html: DOMPurify.sanitize(props[currentIntegration].html) + }} />
64-80: 🛠️ Refactor suggestionAdd keyboard navigation support.
The integration selector should be navigable using keyboard controls.
<div key={name} role="button" + tabIndex={0} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + setCurrentIntegration(key as Integrations); + } + }} onClick={() => setCurrentIntegration(key as Integrations)} className={clsx( "flex cursor-pointer items-center px-3 py-2 font-semibold text-white transition-colors ease-in-out hover:bg-gray-900", key === currentIntegration && "bg-gray-900" )} >apps/web/src/server/services/ConfigService.ts (1)
69-190: 🛠️ Refactor suggestionWrap multiple database operations in a transaction.
The function performs multiple database operations that should be wrapped in a transaction to ensure data consistency.
export async function handlePUT({ config, projectId, userId, }: { userId: string; projectId: string; config: AbbyConfigFile; }) { + return prisma.$transaction(async (tx) => { // ... existing code ... + }); }
🧹 Nitpick comments (45)
apps/web/src/components/EventCounter.tsx (2)
9-21: Consider enhancing accessibility attributes.While the
aria-liveattribute is good for announcing updates, consider adding more descriptive ARIA attributes for better screen reader support.<motion.span initial={{ y: 20, opacity: 0 }} animate={{ y: 0, opacity: 1 }} exit={{ y: -20, opacity: 0 }} transition={{ duration: 0.3 }} aria-live="polite" + role="status" + aria-label="Event counter" >
17-19: Consider adding loading state handling.While the fallback to 0 works, explicitly handling the loading state would improve user experience.
+ const { data, isLoading } = trpc.events.getEventCount.useQuery(); + + if (isLoading) { + return ( + <div className="p-8 text-xl font-bold text-primary mx-auto flex space-x-1"> + <span>Loading...</span> + </div> + ); + } return ( // ... existing JSX );apps/web/src/components/CreateEnvironmentModal.tsx (1)
53-58: Consider enhancing error message handling.While the error handling is good, consider making it more user-friendly by:
- Providing more specific error messages for different error cases
- Extracting error handling logic to a separate function for better maintainability
+ const getErrorMessage = (error: unknown) => { + if (error instanceof TRPCClientError) { + if (error.shape.code === TRPC_ERROR_CODES_BY_KEY.FORBIDDEN) { + return error.message; + } + if (error.shape.code === TRPC_ERROR_CODES_BY_KEY.BAD_REQUEST) { + return "Invalid environment name"; + } + } + return "Error creating environment"; + }; catch (e) { - toast.error( - e instanceof TRPCClientError && - e.shape.code === TRPC_ERROR_CODES_BY_KEY.FORBIDDEN - ? e.message - : "Error creating environment" - ); + toast.error(getErrorMessage(e)); }packages/core/src/validation/index.ts (2)
18-31: Consider whethernullshould be allowed for optional properties.
If you prefer to excludenullin optional properties, remove it from the union types in theInferdefinition.
37-56: Edge case consideration for numeric checks.
Currently,NaNandInfinitypass thetypeof value === "number"check. You may want to add stricter checks if those values are unintended.if (property.type === "number" && typeof value !== "number") { return `Expected number but got ${typeof value}`; } + else if (property.type === "number" && (Number.isNaN(value) || !Number.isFinite(value))) { + return "Expected valid finite number but got NaN or Infinity"; + }packages/core/src/shared/schemas.ts (2)
106-111: Nested record handling inflagValueSchemais valid, but limited.
Consider whether arrays or deeper nesting might also be needed.
144-150: booleanFlagRuleSchema mirrors the same pattern of consistency.
You might unify these definitions with a base schema to avoid repetition.packages/react/src/context.tsx (1)
367-373: updateUserProperties method is a convenient entry point for updating user data.
Consider whether you want to surface validation errors here or in a higher-level error handler.apps/web/src/components/FeatureFlag.tsx (4)
1-8: Remove or refine unused imports.
Lines 1 and 6 include empty imports from"components/ui/popover"and"components/Tooltip". If these modules are no longer needed, please remove the imports to keep the codebase clean, or ensure that any required named exports are actually imported.
104-157: Enhanced modal layout for clarity.
The spacing, label usage, and code blocks in the modal significantly improve readability. Great job making environment details and current/new values clearer. Consider adding minimal validation or user feedback if the "new value" is invalid (e.g., for JSON).
177-201: Memoization and JSON preview logic.
UsinguseMemoforflagValueand creatinggetJSONPreviewhelp with performance and readability. Consider adding unit tests or stories (if using a component library) to verify that parsing edge cases (e.g., deeply nested objects, malformed JSON) are handled gracefully.
204-331: Dynamic rendering for multiple flag types.
The usage ofts-patternand separate blocks for BOOLEAN, NUMBER, STRING, and JSON flags is well-structured. The UI updates, especially the button outlines and code blocks, look consistent. Watch for potential duplication across these blocks—if needed, factor out repeated logic.packages/core/tests/base.test.ts (2)
374-430: Combination logic (AND).
This test verifies that both conditions must be satisfied before the flag is altered. The thorough step-by-step approach, with multiple updates to user properties, is effective. Consider an additional negative test case where the user object is missing one property entire (likeisTest) to confirm robust handling.
490-544: Complex rule set for remote config.
This test elegantly shows multiple rule behaviors (equals, contains) controlling remote config. Adding a test for conflicting rules or verifying fallback logic (in case all rules fail) might further strengthen coverage.packages/core/src/index.ts (2)
139-186: Maintaining local data with rule sets.
Enhanced data initialization forflagsandremoteConfignow includes the optionalruleSet. This design is scalable. If performance becomes an issue with many rule sets, consider memoizing or performing partial evaluations.Also applies to: 190-200
335-345: Inline rule evaluation ingetProjectData.
Forwarding the final flag and remoteConfig values withruleSetincluded helps preserve clarity. This is beneficial for debugging. If you’d like to hide rule sets from external usage, consider a separate private structure.apps/web/abby.config.ts (1)
2-2: Consider adding email format validation.While the string validation for user properties is a good start, consider using a more specific validation for the email field to ensure proper format.
user: { id: validation.string(), - email: validation.string(), + email: validation.string().email(), },Also applies to: 28-31
apps/web/src/components/ui/environment-badge.tsx (1)
10-41: Add ARIA attributes for better accessibility.The component could benefit from improved accessibility by adding appropriate ARIA attributes.
Apply this diff to enhance accessibility:
<div className={cn( "inline-flex items-center gap-2 rounded-md", size === "sm" && "px-2 py-0.5 text-xs", size === "default" && "px-3 py-1 text-sm", size === "lg" && "px-4 py-1.5 text-base", style.bg, style.border, className )} + role="status" + aria-label={`Environment: ${name}`} >apps/web/src/lib/environment-styles.ts (1)
28-40: Consider adding type safety for environment names.The environment name matching could be more type-safe and maintainable.
Consider this improvement:
+type Environment = keyof typeof ENVIRONMENT_COLORS; +const isKnownEnvironment = (name: string): name is Environment => + Object.keys(ENVIRONMENT_COLORS).includes(name); -export const getEnvironmentStyle = (environmentName: string) => { +export const getEnvironmentStyle = (environmentName: string) => { + const normalizedName = environmentName.toLowerCase(); + if (isKnownEnvironment(normalizedName)) { + return ENVIRONMENT_COLORS[normalizedName]; + } const name = environmentName.toLowerCase(); if (name.includes("dev") || name.includes("development")) { return ENVIRONMENT_COLORS.development; } if (name.includes("staging") || name.includes("test")) { return ENVIRONMENT_COLORS.staging; } if (name.includes("prod") || name.includes("production")) { return ENVIRONMENT_COLORS.production; } return ENVIRONMENT_COLORS.default; };apps/web/src/components/settings/Segments.tsx (1)
9-20: Consider using an object map for better maintainability.While the current implementation works correctly, consider refactoring to use an object map for better maintainability:
-function getTypeColor(type: ValidatorType["type"]) { - switch (type) { - case "string": - return "bg-green-500/10 text-green-500"; - case "number": - return "bg-blue-500/10 text-blue-500"; - case "boolean": - return "bg-purple-500/10 text-purple-500"; - default: - return "bg-gray-500/10 text-gray-500"; - } -} +const TYPE_COLORS: Record<ValidatorType["type"] | "default", string> = { + string: "bg-green-500/10 text-green-500", + number: "bg-blue-500/10 text-blue-500", + boolean: "bg-purple-500/10 text-purple-500", + default: "bg-gray-500/10 text-gray-500", +}; + +function getTypeColor(type: ValidatorType["type"]) { + return TYPE_COLORS[type] || TYPE_COLORS.default; +}packages/react/src/StorageService.ts (1)
20-31: Consider applying configurable expiration to all storage services.While the ABStorageService now supports configurable cookie expiration through StorageServiceOptions, FFStorageService and RCStorageService still use the hardcoded DEFAULT_COOKIE_AGE. Consider applying the same pattern to maintain consistency across all storage services.
Example implementation for FFStorageService:
- set(projectId: string, flagName: string, value: string): void { + set(projectId: string, flagName: string, value: string, options?: StorageServiceOptions): void { Cookie.set(getFFStorageKey(projectId, flagName), value, { - expires: DEFAULT_COOKIE_AGE, + expires: options?.expiresInDays ? options.expiresInDays : DEFAULT_COOKIE_AGE, }); }packages/node/tests/koa.test.ts (1)
8-10: Consider using a more descriptive variable name for the server URL.The variable name SERVER_URL could be more descriptive, such as TEST_SERVER_URL or KOA_SERVER_URL, to better indicate its purpose in the test context.
-let SERVER_URL = "http://localhost:"; +let TEST_SERVER_URL = "http://localhost:";packages/core/tests/validation.test.ts (1)
3-91: Enhance test coverage with additional test cases.The test suite provides good coverage of basic validation scenarios, but could be strengthened by adding the following test cases:
- Nested object validation
- Array validation
- Edge cases (empty objects, null values)
Example test case for nested object validation:
+ it("validates nested objects properly", () => { + const addressValidator = { + street: validation.string(), + number: validation.number(), + }; + + const userValidator = { + name: validation.string(), + address: addressValidator, + }; + + const user = { + name: "John", + address: { + street: "Main St", + number: 42, + }, + }; + + expect(validation.validate(userValidator, user)).toEqual({ + value: user, + }); + });packages/remix/src/index.tsx (1)
24-34: Add JSDoc documentation for the User type parameter.The new User type parameter would benefit from documentation explaining its purpose and constraints.
export function createAbby< const FlagName extends string, const TestName extends string, const Tests extends Record<TestName, ABConfig>, const RemoteConfig extends Record<RemoteConfigName, RemoteConfigValueString>, const RemoteConfigName extends Extract<keyof RemoteConfig, string>, + /** + * Type parameter for user properties validation. + * @example + * { + * age: validation.number(), + * name: validation.string(), + * isPremium: validation.boolean() + * } + */ const User extends Record<string, ValidatorType> = Record< string, ValidatorType >, >(apps/web/src/server/services/ConfigService.ts (1)
78-87: Add error handling for user validation.Consider adding a custom error message for better error handling when user validation fails.
- await prisma.project.findFirstOrThrow({ + try { + await prisma.project.findFirstOrThrow({ + where: { + id: projectId, + users: { + some: { + userId, + }, + }, + }, + }); + } catch (error) { + throw new Error(`User ${userId} is not authorized to modify project ${projectId}`); + }apps/web/src/api/routes/v1_project_data.ts (1)
48-48: Consider adding type validation for rule sets.The type casting of
rulestoFlagRuleSetis done without validation, which could lead to runtime errors if the data doesn't match the expected structure.Consider adding runtime type validation using a schema validation library like Zod:
+import { z } from "zod"; + +const FlagRuleSetSchema = z.object({ + // define your rule set schema here +}); + return { name: flagValue.flag.name, value: transformFlagValue(flagValue.value, flagValue.flag.type), - ruleSet: flagValue.ruleSets.at(0)?.rules - ? (flagValue.ruleSets.at(0)?.rules as FlagRuleSet) - : undefined, + ruleSet: flagValue.ruleSets.at(0)?.rules + ? FlagRuleSetSchema.parse(flagValue.ruleSets.at(0)?.rules) + : undefined, };Also applies to: 64-66, 75-77
apps/web/src/components/ProjectSwitcher.tsx (1)
86-90: Consider using semantic HTML elements instead of ignoring the lint rule.The
biome-ignorecomment forlint/a11y/useSemanticElementssuggests that a more semantic HTML element might be appropriate here.Consider using a semantic
selectelement with custom styling instead of abuttonwithrole="combobox":-// biome-ignore lint/a11y/useSemanticElements: <explanation> -<Button - role="combobox" - aria-expanded={open} - aria-label="Select a team" +<Select + aria-expanded={open} + aria-label="Select a team"apps/web/src/components/AddFeatureFlagModal.tsx (1)
31-159: Consider using a form validation library.The current form validation is handled manually. Using a form validation library would provide a more robust solution with built-in validation, error handling, and type safety.
Consider using
react-hook-formwithzodfor form validation:import { useForm } from 'react-hook-form'; import { zodResolver } from '@hookform/resolvers/zod'; import { z } from 'zod'; const flagFormSchema = z.object({ name: z.string().min(1, 'Name is required'), value: z.string().min(1, 'Value is required'), type: z.nativeEnum(FeatureFlagType) }); type FlagFormSchema = z.infer<typeof flagFormSchema>; export function ChangeFlagForm({ /* ... */ }) { const form = useForm<FlagFormSchema>({ resolver: zodResolver(flagFormSchema), defaultValues: initialValues }); return ( <form onSubmit={form.handleSubmit(onChangeHandler)}> {/* ... */} </form> ); }apps/web/src/pages/projects/[projectId]/flags/[flagId].tsx (1)
107-142: Consider adding a link to user definitions setup.The empty state for user definitions provides clear guidance but could be more actionable.
- <Button variant="outline" size="sm" className="mt-4"> + <Button + variant="outline" + size="sm" + className="mt-4" + onClick={() => router.push(`/projects/${router.query.projectId}/user-segments`)} + >apps/web/src/components/flags/FlagRuleEditor.tsx (2)
95-96: Add aria-label for better accessibility.The divider line lacks an accessible label for screen readers.
Apply this diff to improve accessibility:
- <div className="absolute -left-10 top-1/2 -translate-y-1/2 w-6 h-px bg-border" /> + <div + className="absolute -left-10 top-1/2 -translate-y-1/2 w-6 h-px bg-border" + role="separator" + aria-label="Rule separator" + />
108-114: Add error handling for invalid schema.The code silently returns when the schema is invalid. Consider providing feedback to the user.
Apply this diff to add error handling:
- if (!userSchema[value]) return; + if (!userSchema[value]) { + toast.error(`Invalid property: ${value}`); + return; + }Don't forget to import the toast component:
+import { toast } from "react-hot-toast";packages/devtools/src/Devtools.svelte (1)
215-216: Use CSS custom property for font stack.The monospace font stack is duplicated. Consider using a CSS custom property for better maintainability.
Apply this diff to use a CSS custom property:
+ :root { + --font-mono: ui-monospace, "Cascadia Code", "Source Code Pro", Menlo, + Consolas, "DejaVu Sans Mono", monospace; + } #abby-devtools-collapsed { - font-family: ui-monospace, "Cascadia Code", "Source Code Pro", Menlo, - Consolas, "DejaVu Sans Mono", monospace; + font-family: var(--font-mono); } #abby-devtools { - font-family: ui-monospace, "Cascadia Code", "Source Code Pro", Menlo, - Consolas, "DejaVu Sans Mono", monospace; + font-family: var(--font-mono); }Also applies to: 232-233
apps/web/src/server/trpc/router/events.ts (1)
13-273: Consider optimizing data processing.The function processes large amounts of data in memory. Consider these optimizations:
- Move data aggregation to the database level.
- Implement pagination or limit the data range.
- Use streaming for large datasets.
Example database-level aggregation:
// In EventService.ts async function getAggregatedEventsByTestId( testId: string, interval: string, eventType: AbbyEventType ) { const groupByClause = interval === TIME_INTERVAL.DAY ? `DATE_TRUNC('hour', "createdAt") + INTERVAL '${Math.floor(date.hour() / 3) * 3} hour'` : `DATE_TRUNC('day', "createdAt")`; return prisma.$queryRaw` SELECT ${groupByClause} as date, "selectedVariant", SUM("eventCount") as "totalEventCount", SUM("uniqueEventCount") as "uniqueEventCount" FROM "Event" WHERE "testId" = ${testId} AND "type" = ${eventType} GROUP BY date, "selectedVariant" ORDER BY date ASC `; }apps/web/src/components/flags/RuleSetEditor.tsx (2)
20-32: Props interface could be more explicit.Consider extracting the props interface to improve code readability and reusability.
+interface FlagRulesEditorProps { + userSchema: Record<string, ValidatorType>; + flagValue: string; + flagType: FeatureFlagType; + onSave: (ruleSet: FlagRuleSet) => void; + initialData?: FlagRuleSet; +} -export function FlagRulesEditor({ +export function FlagRulesEditor({ userSchema, flagValue, flagType, onSave, initialData, -}: { - userSchema: Record<string, ValidatorType>; - flagValue: string; - flagType: FeatureFlagType; - onSave: (ruleSet: FlagRuleSet) => void; - initialData?: FlagRuleSet; -}) { +}: FlagRulesEditorProps) {
35-58: Consider memoizing rule management functions.The rule management functions are recreated on every render. Consider using
useCallbackto optimize performance.+import { useCallback } from 'react'; -const addRule = () => { +const addRule = useCallback(() => { setRuleSet([ ...ruleSet, { propertyName: "", propertyType: "string", operator: "eq", value: "", thenValue: flagValue, }, ]); -}; +}, [ruleSet, flagValue]); -const updateRule = (index: number, rule: FlagRuleSet[number]) => { +const updateRule = useCallback((index: number, rule: FlagRuleSet[number]) => { const newRuleSet = [...ruleSet]; newRuleSet[index] = rule; setRuleSet(newRuleSet); -}; +}, [ruleSet]); -const removeRule = (index: number) => { +const removeRule = useCallback((index: number) => { const newRuleSet = [...ruleSet]; newRuleSet.splice(index, 1); setRuleSet(newRuleSet); -}; +}, [ruleSet]);apps/web/src/pages/welcome.tsx (1)
146-153: Improve form accessibility with ARIA attributes.While the
htmlForattribute is added, additional ARIA attributes could enhance accessibility further.-<label htmlFor="name"> +<label htmlFor="name" aria-label="Enter your name"> <span>Name</span> <Input name="name" + id="name" + aria-required="true" + aria-describedby="name-description" value={name} onChange={(e) => setName(e.target.value)} /> + <span id="name-description" className="sr-only"> + Please enter your full name + </span> </label>apps/web/src/server/trpc/router/project.ts (1)
45-45: Consider selective loading of user segments.Including
userSegmentsin every project query might impact performance. Consider implementing selective loading based on usage requirements.-userSegments: true, +userSegments: input.includeUserSegments ?? false,And update the input schema:
.input(z.object({ projectId: z.string(), includeUserSegments: z.boolean().optional() }))apps/web/src/components/FlagPage.tsx (2)
375-376: Consider implementing virtualization for large lists.The grid layout with auto-fill might cause performance issues with a large number of flags. Consider implementing virtualization.
+import { VirtualGrid } from 'react-virtual-grid'; -<div className="grid grid-cols-[repeat(auto-fill,minmax(250px,1fr))] gap-4"> +<VirtualGrid + itemCount={currentFlag.values.length} + itemSize={250} + overscanCount={2} +> {currentFlag.values .sort((a, b) => a.environment.sortIndex - b.environment.sortIndex) .map((flagValue) => (Also applies to: 381-382
391-404: Enhance button accessibility with keyboard navigation.The Configure button could benefit from improved keyboard navigation and ARIA attributes.
<Button variant="ghost" size="sm" className="px-2 text-xs h-7 bg-muted/50 hover:bg-muted group/configure" + role="link" + aria-label={`Configure flag for ${flagValue.environment.name} environment`} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + router.push(`/projects/${projectId}/flags/${flagValue.id}`); + } + }} onClick={() => router.push( `/projects/${projectId}/flags/${flagValue.id}` ) } >apps/web/prisma/migrations/20250131071445_add_flag_rulesets/migration.sql (2)
1-14: UserSegment Table Definition Review.
TheUserSegmenttable is defined with appropriate columns and indexes. As an optional improvement, consider adding anON UPDATE CURRENT_TIMESTAMP(3)clause for theupdatedAtcolumn if automatic update tracking is desired.
15-28: FlagRuleSet Table Definition Review.
TheFlagRuleSettable is set up correctly with necessary indexes and constraints. Optionally, you might want to include more robust mechanisms for auto-updating theupdatedAtfield or enforcing relational integrity with foreign keys where applicable.apps/docs/pages/feature-flags.mdx (2)
32-34: Nitpick on Redundant Phrasing in Rule Groups DescriptionThe description for rule groups ("Complex logic combining multiple conditions with AND/OR operators") may be streamlined to avoid potential duplication or verbosity. Consider rephrasing it as “Complex logic to group conditions with AND/OR operators” for enhanced clarity.
🧰 Tools
🪛 LanguageTool
[duplication] ~33-~33: Possible typo: you repeated a word.
Context: ...mbining multiple conditions with AND/OR operators - Operators: Support for various comparisons: -...(ENGLISH_WORD_REPEAT_RULE)
64-67: Stylistic Suggestion: Consistent HyphenationIn the bullet point "Target specific user segments with custom rules," consider hyphenating “Target-specific” to improve readability.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~67-~67: When ‘Target-specific’ is used as a modifier, it is usually spelled with a hyphen.
Context: ...ures gradually based on usage metrics - Target specific user segments with custom rules(SPECIFIC_HYPHEN)
apps/docs/pages/user-segments.mdx (1)
49-65: Configuration of Feature Flag RulesThe instructions for setting up rules in the Abby dashboard are clear and actionable. For the rule regarding long-time users (line 63), consider rephrasing to something more concise like “Create a rule where accountAgeDays > 30” to enhance clarity.
🧰 Tools
🪛 LanguageTool
[style] ~63-~63: This phrasing could be wordy, so try replacing it with something more concise.
Context: ...: - Create a rule where accountAgeDays is greater than 30 These rules are evaluated in order,...(MORE_THAN_EXCEEDS)
apps/web/prisma/schema.prisma (1)
1-365: Overall Prisma Schema EnhancementsThe schema updates—including new models and relationships—are a significant improvement that aligns with the core objective of enhancing user functionality. Consider adding inline comments to describe the purpose of the new models for future maintainers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (67)
apps/docs/pages/_meta.json(1 hunks)apps/docs/pages/feature-flags.mdx(1 hunks)apps/docs/pages/reference/_meta.json(1 hunks)apps/docs/pages/reference/nextjs.mdx(3 hunks)apps/docs/pages/reference/operators.mdx(1 hunks)apps/docs/pages/reference/react.mdx(5 hunks)apps/docs/pages/reference/remix.mdx(2 hunks)apps/docs/pages/user-segments.mdx(1 hunks)apps/web/abby.config.ts(2 hunks)apps/web/package.json(1 hunks)apps/web/prisma/migrations/20250131071445_add_flag_rulesets/migration.sql(1 hunks)apps/web/prisma/schema.prisma(3 hunks)apps/web/src/api/routes/v1_project_data.ts(4 hunks)apps/web/src/components/AddFeatureFlagModal.tsx(5 hunks)apps/web/src/components/Avatar.tsx(1 hunks)apps/web/src/components/CodeSnippet.tsx(1 hunks)apps/web/src/components/CreateAPIKeyModal.tsx(1 hunks)apps/web/src/components/CreateEnvironmentModal.tsx(1 hunks)apps/web/src/components/EventCounter.tsx(1 hunks)apps/web/src/components/FeatureFlag.tsx(4 hunks)apps/web/src/components/FlagPage.tsx(5 hunks)apps/web/src/components/Layout.tsx(3 hunks)apps/web/src/components/Progress.tsx(1 hunks)apps/web/src/components/ProjectSwitcher.tsx(1 hunks)apps/web/src/components/Test/CreateTestSection.tsx(1 hunks)apps/web/src/components/flags/FlagRuleEditor.tsx(1 hunks)apps/web/src/components/flags/RuleSetEditor.tsx(1 hunks)apps/web/src/components/settings/Integrations.tsx(1 hunks)apps/web/src/components/settings/Segments.tsx(1 hunks)apps/web/src/components/ui/environment-badge.tsx(1 hunks)apps/web/src/lib/abby.tsx(1 hunks)apps/web/src/lib/environment-styles.ts(1 hunks)apps/web/src/pages/_app.tsx(1 hunks)apps/web/src/pages/_error.jsx(1 hunks)apps/web/src/pages/index.tsx(1 hunks)apps/web/src/pages/projects/[projectId]/environments.tsx(7 hunks)apps/web/src/pages/projects/[projectId]/flags/[flagId].tsx(1 hunks)apps/web/src/pages/projects/[projectId]/settings.tsx(5 hunks)apps/web/src/pages/welcome.tsx(1 hunks)apps/web/src/server/services/ConfigService.ts(2 hunks)apps/web/src/server/services/EventService.ts(1 hunks)apps/web/src/server/trpc/router/events.ts(1 hunks)apps/web/src/server/trpc/router/flags.ts(2 hunks)apps/web/src/server/trpc/router/project.ts(1 hunks)package.json(1 hunks)packages/angular/src/lib/abby.service.ts(5 hunks)packages/core/package.json(1 hunks)packages/core/src/defineConfig.ts(2 hunks)packages/core/src/index.ts(13 hunks)packages/core/src/shared/schemas.ts(3 hunks)packages/core/src/shared/types.ts(2 hunks)packages/core/src/validation/index.ts(1 hunks)packages/core/tests/base.test.ts(2 hunks)packages/core/tests/defineConfig.test.ts(3 hunks)packages/core/tests/types.test.ts(2 hunks)packages/core/tests/validation.test.ts(1 hunks)packages/devtools/src/Devtools.svelte(7 hunks)packages/next/src/index.tsx(4 hunks)packages/next/src/withAbby.tsx(1 hunks)packages/node/package.json(1 hunks)packages/node/tests/express.test.ts(1 hunks)packages/node/tests/koa.test.ts(3 hunks)packages/node/tests/node.test.ts(1 hunks)packages/react/src/StorageService.ts(1 hunks)packages/react/src/context.tsx(8 hunks)packages/remix/src/index.tsx(4 hunks)packages/tsconfig/react-library.json(1 hunks)
✅ Files skipped from review due to trivial changes (6)
- apps/web/src/components/Progress.tsx
- apps/web/src/pages/_error.jsx
- apps/web/src/pages/_app.tsx
- apps/web/src/pages/index.tsx
- apps/web/src/components/Layout.tsx
- apps/web/src/server/services/EventService.ts
🧰 Additional context used
🪛 LanguageTool
apps/docs/pages/feature-flags.mdx
[duplication] ~33-~33: Possible typo: you repeated a word.
Context: ...mbining multiple conditions with AND/OR operators - Operators: Support for various comparisons: -...
(ENGLISH_WORD_REPEAT_RULE)
[uncategorized] ~67-~67: When ‘Target-specific’ is used as a modifier, it is usually spelled with a hyphen.
Context: ...ures gradually based on usage metrics - Target specific user segments with custom rules
(SPECIFIC_HYPHEN)
apps/docs/pages/user-segments.mdx
[style] ~63-~63: This phrasing could be wordy, so try replacing it with something more concise.
Context: ...: - Create a rule where accountAgeDays is greater than 30 These rules are evaluated in order,...
(MORE_THAN_EXCEEDS)
🔇 Additional comments (114)
apps/web/src/components/EventCounter.tsx (2)
1-2: LGTM! Clean import structure.The imports are well-organized and properly consolidated.
4-6: LGTM! Well-structured component.The component follows React best practices with proper hook usage for data fetching.
apps/web/src/components/CreateEnvironmentModal.tsx (1)
62-64: Great accessibility improvements!The label and input are now properly associated using matching
htmlForandnameattributes, improving the experience for screen reader users.Also applies to: 66-66
packages/core/src/validation/index.ts (5)
1-2: No issues with the import/export statements.
4-6: Basic validator type definitions look good.
8-10: Factory functions for string, number, and boolean are straightforward.
12-17: Optional validator approach is clear and concise.
32-36: ValidationErrors type adequately captures error details.packages/core/src/shared/schemas.ts (11)
26-33: Validator type schema definition is clear.
35-35: Exporting theValidatorTypegives external modules easy access.
81-81: Optional user property in abbyConfigSchema is well-structured.
113-127: stringFlagRuleSchema definition is solid.
129-145: numberFlagRuleSchema definition is comprehensive.
152-156: flagRuleSchema usingdiscriminatedUnionis a good approach.
158-162: subFlagRuleSchema works well for nested logic.
164-172: flagRulesSetSchema introduces logical operatorsand/orsensibly.
174-180: Type exports (FlagRuleSet, FlagRule, SubFlagRule) clearly map to schemas.
181-203: getOperatorsForType function is well-structured and easy to extend.
205-233: getDisplayNameForOperator is a neat help for UI readability.packages/react/src/context.tsx (10)
7-7: Importing ValidatorType from core is appropriate.
11-11: Importing Infer from the validation module is consistent.
45-48: Generic parameter for User is flexible.
55-57: Inclusion of RemoteConfig and User in the AbbyConfig ensures type safety.
65-67: Extending Abby with User confirms typed user properties in the constructor.
205-205: Retrieving the feature flag's value is straightforward.
215-215: useFeatureFlags also neatly accesses each flag's value.
227-227: Remote config value retrieval is consistent with feature flags.
282-283: Typecast to RemoteConfigValueStringToType is accurate for typed access.
389-389: Exposing updateUserProperties from createAbby fosters easy usage.apps/web/src/components/FeatureFlag.tsx (4)
22-39: Exported function complements decoupled usage.
This arrow function for describing history events appears well-structured. Exporting it likely aids reuse in other files. Consider adding brief documentation if used across modules, clarifying how or when it should be invoked.
45-61: Prop addition aligns modal with environment context.
IntroducingenvironmentNamein theConfirmUpdateModalprops, along with usage in line 68, is a good way to contextualize the environment for the user. Ensure that all invocations ofConfirmUpdateModalpass the correct environment name to avoid undefined references.Also applies to: 68-68
162-169: Optionalminimalprop promotes flexible UI.
Allowing a minimal rendering with a Boolean prop is a clean approach. Ensure consistent naming and usage across the codebase to avoid confusion with other "compact" or "small" props.
333-343: Good integration withConfirmUpdateModal.
Linking the modal state to the button ensures a straightforward update flow. Double-check that the default values in the modal match the storedflagValueto prevent confusion for the user if states get out of sync.packages/core/tests/base.test.ts (5)
3-3: Validation library import.
The import ofvalidationis important for new user property checks below. Ensure the same instance is used across other test files to maintain consistency if you rely on a shared global state or configuration in these validators.
358-373: Test for validating user properties.
This test correctly ensures that updating user properties with an invalid type throws the expected error message. Great coverage for improper usage. Consider adding more test cases for other property types or for partially valid payloads if relevant.
432-488: Disjunctive logic (OR).
Nice coverage of using OR-based rule sets. The repeated user property updates effectively demonstrate how the feature flag toggles. If feasible, add an explicit test for multiple OR conditions to ensure it handles more than two rules.
546-599: Ordered rule sets.
Verifying order withcontainsprecedingeqcan be subtle. Good demonstration here. Remember to highlight in documentation that the first matching rule short-circuits the evaluation, as this is critical for user understanding.
601-671: Stacked rule sets.
Your test ensuresandcondition handling in combination with simpler single rules. The transitions between different user property values help confirm the system’s intended behavior. Consider merging or referencing these patterns in the documentation for clarity.packages/core/src/index.ts (5)
6-7: Additional imports and validation usage.
The inclusion ofFlagRuleSetandvalidationis essential for the new user rules. Just ensure that all references to these types and functions remain consistent, especially if you expand the property evaluation logic further.Also applies to: 22-22
100-104: Expanded generics to accommodate user definition.
Introducing theUsertype inAbbyConfigis a neat approach to strongly type user properties. It’s flexible enough for diverse validation rules. Double-check that no unintended constraints appear if a user wants to skip providing user properties.Also applies to: 120-120, 131-133
287-298: Converting response to local data with rule evaluation.
Immediately runningthis.evaluateUserPropertiesensures correct initial values. If the user updates properties afterwards, the re-evaluation is triggered elsewhere—just confirm that logic is consistent across the codebase so no stale data remains.Also applies to: 300-311
503-517: Remote config rule evaluation.
In development mode, overrides still remain top priority. The subsequent fallback toevaluateUserPropertiesis a clean design. Validate that all property types (STRING, NUMBER, etc.) are covered in real usage.
826-979: Core user rules evaluation withand,or, and single conditions.
TheevaluateUserPropertiesfunction is comprehensive and covers diverse operators (e.g.,contains,gt). Watch for potential expansions (like multi-value conditions or other data types). Each newly added filter or operator should come with a unit test, as done in the test suite.apps/web/src/lib/abby.tsx (1)
16-16: LGTM! The new export aligns with the PR objective.The addition of
updateUserPropertiesto the exports is consistent with the PR's objective of adding user functionality.packages/core/src/shared/types.ts (1)
1-1: LGTM! Well-structured type enhancements.The changes enhance the type system to support user-based feature flag rules:
- The
FlagRuleSettype is imported and used consistently.- The changes are symmetric across both
flagsandremoteConfigarrays.Also applies to: 13-22
packages/node/tests/node.test.ts (1)
14-14: LGTM! Appropriate test configuration.The addition of
currentEnvironment: "development"is appropriate for tests and aligns with similar modifications in other test files.apps/web/src/components/ui/environment-badge.tsx (1)
4-8: LGTM! Well-defined props interface.The props interface is clear and type-safe with appropriate optional properties.
packages/core/src/defineConfig.ts (1)
14-34: LGTM! Enhanced type safety with user property validation.The addition of the
Usertype parameter with defaultRecord<string, ValidatorType>improves type safety while maintaining backward compatibility.apps/web/src/lib/environment-styles.ts (1)
1-26: LGTM! Well-organized color configurations.The color configurations are well-structured and follow a consistent pattern for each environment.
apps/web/src/components/Avatar.tsx (1)
21-21: Add explanation for the accessibility lint ignore.Please provide an explanation for why the accessibility lint rule is being ignored. This helps future maintainers understand the reasoning behind this decision.
Update the comment to include the explanation:
-// biome-ignore lint/a11y/useSemanticElements: <explanation> +// biome-ignore lint/a11y/useSemanticElements: Avatar root is interactive and styled as a buttonpackages/core/tests/defineConfig.test.ts (3)
3-3: LGTM!The import statement for the validation module is correctly added and aligns with the new user validation functionality.
21-25: LGTM!The user configuration is well-structured with appropriate validation rules for each field:
nameuses string validationageuses number validationisDeveloperuses boolean validation
41-45: LGTM!The type expectation for
updateUserPropertiescorrectly defines all fields as optional, allowing for partial updates of user properties.apps/web/src/components/CreateAPIKeyModal.tsx (1)
44-48: LGTM! Accessibility improvements enhance form usability.The label and input are correctly associated using matching
htmlForandnameattributes, improving screen reader support and following WCAG guidelines.packages/node/tests/express.test.ts (1)
17-17: LGTM!The addition of
currentEnvironment: "development"is appropriate for the test context and aligns with similar changes in other test files.apps/web/src/components/settings/Segments.tsx (3)
1-7: LGTM!The imports and interface definition are well-structured and provide clear type safety for the component.
22-24: Add runtime validation for schema type cast.The type cast of
segment.schemacould be unsafe if the schema is invalid.Consider adding runtime validation:
- const schema = segment.schema as Record<string, ValidatorType>; + const schema = segment.schema as Record<string, ValidatorType>; + if (!schema || typeof schema !== 'object') { + throw new Error('Invalid segment schema'); + }
25-55: LGTM! Well-structured and responsive UI implementation.The render logic is well-implemented with:
- Appropriate use of Card components
- Responsive design with hover states
- Proper handling of optional fields
- Consistent styling and transitions
packages/core/tests/types.test.ts (1)
49-68: LGTM! Good test coverage for user types.The test suite effectively validates the type correctness of user properties and the updateUserProperties method. The use of validation functions for each property type ensures type safety.
packages/next/src/withAbby.tsx (1)
64-64: LGTM! Improved operator precedence clarity.The added parentheses make the nullish coalescing operation more explicit without changing the behavior.
packages/node/tests/koa.test.ts (1)
57-61: LGTM! Good use of dynamic port assignment.Using get-port for dynamic port assignment is a good practice as it prevents port conflicts during parallel test execution.
apps/web/src/api/routes/v1_project_data.ts (1)
3-3: LGTM!The import of
FlagRuleSettype from@tryabby/core/schemais correctly added.apps/web/src/components/settings/Integrations.tsx (1)
78-80: Consider using semantic HTML elements instead of ignoring the lint rule.Similar to ProjectSwitcher.tsx, the
biome-ignorecomment forlint/a11y/useSemanticElementssuggests that a more semantic HTML element might be appropriate here.apps/web/src/components/AddFeatureFlagModal.tsx (1)
64-80: LGTM! Improved form layout and accessibility.The changes enhance the form's accessibility and layout by:
- Using semantic Label components
- Implementing a grid layout for better organization
- Using design system tokens for error message styling
apps/web/src/components/Test/CreateTestSection.tsx (1)
137-143: LGTM! Accessibility improvements.The addition of
htmlForandnameattributes properly associates the label with its input field, improving accessibility for screen readers.apps/web/src/pages/projects/[projectId]/flags/[flagId].tsx (2)
23-42: LGTM! Well-structured error handling and loading states.The component properly handles loading and error states, providing appropriate feedback to users.
144-185: LGTM! Well-implemented history section.The history section effectively displays changes with user avatars, timestamps, and proper overflow handling.
packages/next/src/index.tsx (1)
29-32: LGTM! Enhanced type safety with user validation.The addition of the
Usergeneric parameter with proper defaults and theupdateUserPropertiesmethod improves type safety while maintaining backward compatibility.Also applies to: 55-55, 96-96
packages/angular/src/lib/abby.service.ts (1)
47-51: LGTM! Enhanced type safety with rule sets.The changes improve type safety by:
- Adding proper typing for rule sets in flags and remote config
- Updating observable types to include rule sets
- Maintaining consistency across the service
Also applies to: 98-100, 237-239, 251-253
apps/web/src/components/flags/FlagRuleEditor.tsx (1)
23-76: LGTM! Well-structured component with appropriate input types.The component effectively handles different flag types with appropriate input components and value handling.
packages/devtools/src/Devtools.svelte (1)
16-20: LGTM! Improved readability with multiline formatting.The position property is now more readable with each option on a separate line.
apps/web/src/pages/projects/[projectId]/environments.tsx (2)
108-147: LGTM! Well-structured modal with proper error handling.The component effectively handles environment deletion with appropriate feedback and error handling.
149-298: LGTM! Well-structured page with proper state handling and UX.The component effectively manages environments with appropriate loading states, empty states, and user interactions.
apps/web/src/server/trpc/router/events.ts (1)
334-345: LGTM! Efficient implementation with appropriate caching.The function effectively calculates total event count with proper memoization and parallel processing.
apps/web/src/server/trpc/router/flags.ts (1)
516-578: LGTM! Well-structured rule set management.The procedure includes:
- Proper authorization checks
- Validation using flagRulesSetSchema
- Cache invalidation after updates
- Handles both new and existing rule sets
apps/web/src/pages/projects/[projectId]/settings.tsx (2)
156-161: LGTM! Improved form accessibility.Good addition of htmlFor and name attributes to improve form accessibility.
241-278: LGTM! Well-structured user segments UI.The new Segments tab provides:
- Clear explanations of user segments
- Proper conditional rendering
- Helpful documentation links
- Good use of components for segment display
apps/docs/pages/reference/_meta.json (1)
8-9: LGTM! Clean metadata addition.The new operators entry is properly formatted and maintains consistency with existing entries.
packages/tsconfig/react-library.json (1)
9-9:Details
✅ Verification successful
Verify TypeScript version compatibility for bundler moduleResolution.
The bundler moduleResolution option requires TypeScript 5.0 or later.
🌐 Web query:
What TypeScript version introduced the "bundler" moduleResolution option and what are its implications?💡 Result:
TypeScript introduced the
"bundler"module resolution option in version 5.0 (released March 16, 2023)[6][8]. This addition aimed to better model how modern bundlers resolve modules while supporting ECMAScript Modules (ESM) and CommonJS hybrid workflows.Key Implications of
"moduleResolution": "bundler"
Bundler-First Compatibility
- Allows extensionless imports (e.g.,
import "./utils"instead of"./utils.js"), trusting bundlers to resolve them[1][6].- Supports
package.json"exports"and"imports"fields by default while relaxing Node.js' strict file extension requirements[4][10].Runtime Compatibility Tradeoffs
- Code compiled with
bundlermay fail in Node.js environments if imports lack explicit.jsextensions[1]. For example:// TS: ✅ Works with bundlers export * from "./utils"; // JS Output: ❌ Fails in Node.js without "./utils.js"- Requires pairing with
"module": "esnext"(TS 5.0)[6] or"module": "preserve"(TS 5.4)[3] to avoid transforming ESM imports to CommonJS.Declaration File Challenges
- Declaration files must match bundled output exactly[1]. Unbundled
.d.tsfiles with extensionless imports will cause errors for Node.js users[1].Hybrid Resolution Support
- From TS 5.4+, works with
require()via"module": "preserve", enabling:This better aligns with Bun-like runtimes and complex bundler workflows[3].// ✅ Allowed in TS 5.4+ import myModule = require("module/path");When to Use It
- Recommended for applications using bundlers like Webpack/Vite/Rollup[1][7].
- Avoid for libraries unless you bundle declarations and test Node.js compatibility[1][6].
- Alternative: Use
"nodenext"/"node16"for stricter Node.js ESM compliance[4][6].The
bundlermode reflects TypeScript's shift to prioritize modern tooling workflows while requiring developers to understand their deployment targets' module resolution rules[6][10].Citations:
- 1: https://www.typescriptlang.org/docs/handbook/modules/guides/choosing-compiler-options.html
- 2: https://www.youtube.com/watch?v=MEl2R7mEAP8
- 3: https://devblogs.microsoft.com/typescript/announcing-typescript-5-4/
- 4: https://www.typescriptlang.org/tsconfig/moduleResolution.html
- 5: https://dev.to/a0viedo/nodejs-typescript-and-esm-it-doesnt-have-to-be-painful-438e
- 6: https://devblogs.microsoft.com/typescript/announcing-typescript-5-0/
- 7: https://www.totaltypescript.com/workshops/typescript-pro-essentials/configuring-typescript/the-moduleresolution-option-in-tsconfigjson
- 8: https://javascript-conference.com/blog/typescript-5/
- 9: https://github.com/IanVS/ts-module-resolution-examples
- 10: https://www.typescriptlang.org/docs/handbook/modules/theory.html
Ensure TypeScript 5.0+ Compatibility for Bundler Module Resolution
The
"bundler"option inmoduleResolutionwas introduced in TypeScript 5.0 and is tailored for modern bundler workflows, supporting extensionless imports and hybrid ESM/CommonJS patterns. Please verify that:
- The project is using TypeScript 5.0 or later.
- Appropriate module settings (such as
"module": "esnext"or"module": "preserve") are in place to support this option.- Any runtime environments (e.g., Node.js) are configured to handle potential issues with extensionless imports.
apps/docs/pages/_meta.json (1)
19-19: New Documentation Entry for User Segments Added.
The addition of the"user-segments": "User Segments"key aligns well with the enhanced documentation for user segmentation. Ensure that corresponding pages (e.g.,user-segments.mdx) reference this key for consistency.package.json (1)
23-24: Dependency Version Bump for @biomejs/biome.
The update from the previous version to"1.9.4"should support the new feature enhancements. Please verify that this version change is compatible with your existing tooling (e.g., lint and format workflows).packages/node/package.json (1)
37-37: Addition of get-port for Dynamic Port Allocation.
Introducing"get-port": "^7.1.0"enhances test flexibility by enabling dynamic port assignment. Ensure that your tests (likely inpackages/node/tests/koa.test.ts) leverage this dependency instead of hardcoding port values.packages/core/package.json (1)
22-32: Expanded Exports for Validation and Schema Management.
The newly added export entries for"./validation"and"./schema"extend the module's API to support dynamic user property updates and validation workflows. Confirm that the corresponding build outputs (i.e., the generated files indist/validation/anddist/shared/) are correctly produced during the build process.apps/docs/pages/feature-flags.mdx (4)
9-10: Introduced "Basic Usage" SectionThe new "Basic Usage" section clearly explains that when a feature flag is toggled for an environment, all users see the same value. This improves the overall clarity of the documentation.
13-14: Addition of User Segments & Conditional Rules SectionThe new section on "User Segments & Conditional Rules" effectively introduces how user properties can influence flag evaluations. This addition aligns well with the broader user functionality objectives.
47-54: Clear Code Example for Updating User PropertiesThe TypeScript snippet demonstrating the use of
abby.updateUserPropertiesis well-constructed and clearly illustrates how user properties are updated.
56-60: Clear Explanation of Feature Flag Evaluation ProcessThe step-by-step explanation of how Abby evaluates feature flags is concise and easy to follow.
apps/docs/pages/user-segments.mdx (5)
1-3: Introduction is Clear and InformativeThe opening lines provide a good overview of the purpose and scope of this guide on user segments.
5-12: Well-Outlined Scenario for Premium Feature AccessThe scenario and accompanying bullet list clearly articulate the use case for premium feature access, setting a solid context for the implementation details that follow.
13-34: Environment Configuration for User PropertiesThe configuration snippet (using
abby.config.ts) precisely outlines the new user properties, including their validations. Make sure that thedefineConfigfunction and necessary imports are available in the surrounding codebase.
36-47: Clear Code Example for Updating User PropertiesThe example showing how to update user properties is straightforward and clearly indicates when these updates should occur (e.g., on user login or subscription changes).
67-81: Usage Example in ComponentsThe React component example (using
useFeatureFlag) effectively demonstrates how to conditionally render based on user access. This practical example complements the earlier documentation very well.apps/docs/pages/reference/operators.mdx (7)
1-2: Introduction to Rule OperatorsThe opening section succinctly introduces the various operators available for configuring feature flags.
5-16: Well-Formatted Table for String OperatorsThe table detailing string operators is clearly structured and provides useful examples, making it easy for the reader to understand each operator’s use case.
17-27: Number Operators Section is ClearThe number operators table is well-organized with clear descriptions and examples.
28-33: Boolean Operators Section is ConciseThe presentation of boolean operators is succinct and effective.
34-42: Rule Groups DocumentationThe explanation of how to combine rules using logical operators (AND/OR) is well presented and aids in understanding more complex rule configurations.
43-94: Comprehensive Example Rule SetsThe detailed TypeScript examples—including single rules and grouped rules—provide a solid reference for developers looking to implement these patterns in their applications.
96-100: Steps for Rule Evaluation ClearThe final section that outlines how rules are evaluated reinforces the overall documentation and clarifies the evaluation order.
apps/web/package.json (2)
6-6: Script Adjustment: New Prebuild ScriptAdding the
"prebuild": "pnpm run db:generate"script ensures that database generation runs before the build process, which is a useful optimization to catch potential issues early.
6-13: Removal of Postinstall ScriptThe removal of the
"postinstall": "pnpm run db:generate"script streamlines the workflow by eliminating redundant database generation. Ensure that this change is well-documented in your release notes for clarity.apps/web/prisma/schema.prisma (3)
112-112: Project Model Update: Adding User Segments RelationThe addition of the
userSegments UserSegment[]field to the Project model appropriately establishes a relationship with the new UserSegment model, facilitating better organization of project-specific user segments.
338-350: Introduction of UserSegment ModelThe newly added
UserSegmentmodel is well-defined with necessary fields, including proper relation mappings, unique constraints, and indexes. This addition supports the enhanced user functionality.
352-364: Introduction of FlagRuleSet ModelThe
FlagRuleSetmodel is effectively introduced to manage feature flag rules, including essential fields and constraints. Ensure that your application logic validates therulesJSON structure appropriately.apps/docs/pages/reference/remix.mdx (3)
9-18: New 'user' Parameter in Configuration Table Added
The new table row for theuserproperty is a clear and concise addition. It correctly documents that this property is optional and provides an appropriate description for targeting feature flags based on user attributes.
84-102: Addition of the 'user' Section with Code Example
The inserted "#### user" section and its accompanying code block effectively demonstrate how to configure user properties using validation helpers from@tryabby/core/validation. The example is well-structured and provides clarity on optional fields.
103-142: Comprehensive Documentation for updateUserProperties
The new "#### updateUserProperties" section thoroughly explains the purpose of the function, its validation process, and includes examples for valid, invalid, and partial updates. This level of detail will help users understand how to update user properties safely.apps/docs/pages/reference/react.mdx (4)
9-19: Incorporation of 'user' Parameter in the Parameter Table
The addition of theuserfield to the parameter table is well-executed. The description clearly states its optional nature and its use for feature flag targeting. This change is consistent with similar documentation updates in other parts of the project.
55-73: New 'user' Section with Validation Example Added
The "#### user" section now provides a useful code snippet that shows how to use validation helpers to configure user properties. The example is clear, concise, and consistent with the project’s established patterns.
138-155: Example Demonstrating User-based Feature Flag Usage
The newly added "##### Example with User Properties" section foruseFeatureFlagexplains how user attributes can influence flag evaluation. This additional example enhances clarity for developers and demonstrates practical usage within a React component.
286-326: Thorough 'updateUserProperties' Documentation and Examples
The detailed "#### updateUserProperties" section documents the behavior of the update function clearly, outlining validation, error handling, and support for partial updates. The provided examples are comprehensive and illustrate both valid and erroneous updates effectively.apps/docs/pages/reference/nextjs.mdx (3)
9-18: Addition of the 'user' Field in the Parameter Table
The inclusion of the newuserproperty in the configuration table is well done. The row clearly states its optional nature and purpose, aligning with the updates made in the other documentation files.
84-102: New 'user' Section Provides Clear Configuration Guidance
The "#### user" section now includes a practical code example using@tryabby/core/validation. This addition is clear and will be valuable for users configuring thecreateAbbyfunction with user properties.
354-394: Detailed Documentation for updateUserProperties Function
This new section explains theupdateUserPropertiesfunction in depth, including the validation behavior and error throwing for invalid types. The examples cover various update scenarios, which enhances usability and understanding.
| export function validate<T extends Record<string, ValidatorType>>( | ||
| userValidator: T, | ||
| user: Record<string, unknown> | ||
| ): | ||
| | { | ||
| errors: ValidationErrors; | ||
| value?: never; | ||
| } | ||
| | { | ||
| errors?: never; | ||
| value: { | ||
| [key in keyof T]: Infer<T[keyof T]>; | ||
| }; | ||
| } { | ||
| const returnObject: { | ||
| [key in keyof T]: Infer<T[keyof T]>; | ||
| } = {} as any; | ||
| const errors: ValidationErrors = []; | ||
| for (const key in userValidator) { | ||
| const validator = userValidator[key]; | ||
| const value = user[key]; | ||
|
|
||
| const error = validateProperty(validator, value); | ||
| if (error) { | ||
| errors.push({ | ||
| property: key, | ||
| message: error, | ||
| }); | ||
| continue; | ||
| } | ||
|
|
||
| returnObject[key] = value as Infer<typeof validator>; | ||
| } | ||
| if (errors.length > 0) { | ||
| return { errors }; | ||
| } | ||
| return { value: returnObject }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refine type assignment for returnObject.
Instead of using value as Infer<typeof validator>, consider indexing T directly to ensure property-specific types and avoid potential mismatch:
- returnObject[key] = value as Infer<typeof validator>;
+ returnObject[key] = value as Infer<T[typeof key]>;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function validate<T extends Record<string, ValidatorType>>( | |
| userValidator: T, | |
| user: Record<string, unknown> | |
| ): | |
| | { | |
| errors: ValidationErrors; | |
| value?: never; | |
| } | |
| | { | |
| errors?: never; | |
| value: { | |
| [key in keyof T]: Infer<T[keyof T]>; | |
| }; | |
| } { | |
| const returnObject: { | |
| [key in keyof T]: Infer<T[keyof T]>; | |
| } = {} as any; | |
| const errors: ValidationErrors = []; | |
| for (const key in userValidator) { | |
| const validator = userValidator[key]; | |
| const value = user[key]; | |
| const error = validateProperty(validator, value); | |
| if (error) { | |
| errors.push({ | |
| property: key, | |
| message: error, | |
| }); | |
| continue; | |
| } | |
| returnObject[key] = value as Infer<typeof validator>; | |
| } | |
| if (errors.length > 0) { | |
| return { errors }; | |
| } | |
| return { value: returnObject }; | |
| } | |
| export function validate<T extends Record<string, ValidatorType>>( | |
| userValidator: T, | |
| user: Record<string, unknown> | |
| ): | |
| | { | |
| errors: ValidationErrors; | |
| value?: never; | |
| } | |
| | { | |
| errors?: never; | |
| value: { | |
| [key in keyof T]: Infer<T[keyof T]>; | |
| }; | |
| } { | |
| const returnObject: { | |
| [key in keyof T]: Infer<T[keyof T]>; | |
| } = {} as any; | |
| const errors: ValidationErrors = []; | |
| for (const key in userValidator) { | |
| const validator = userValidator[key]; | |
| const value = user[key]; | |
| const error = validateProperty(validator, value); | |
| if (error) { | |
| errors.push({ | |
| property: key, | |
| message: error, | |
| }); | |
| continue; | |
| } | |
| returnObject[key] = value as Infer<T[typeof key]>; | |
| } | |
| if (errors.length > 0) { | |
| return { errors }; | |
| } | |
| return { value: returnObject }; | |
| } |
| updateUserProperties( | ||
| user: Partial<{ | ||
| -readonly [K in keyof User]: validation.Infer<User[K]>; | ||
| }> | ||
| ) { | ||
| if (!this.config.user) { | ||
| throw new Error("User properties are not defined in the config"); | ||
| } | ||
|
|
||
| const validationErrors: validation.ValidationErrors = []; | ||
| // validate only the properties that are given as an argument | ||
| for (const [key, value] of Object.entries(user)) { | ||
| const validationErrorMessage = validation.validateProperty( | ||
| this.config.user[key], | ||
| value | ||
| ); | ||
| if (validationErrorMessage) { | ||
| validationErrors.push({ | ||
| property: key, | ||
| message: validationErrorMessage, | ||
| }); | ||
| } | ||
| } | ||
| if (validationErrors.length > 0) { | ||
| throw new Error( | ||
| validationErrors.map((e) => `[${e.property}]: ${e.message}`).join(", ") | ||
| ); | ||
| } | ||
|
|
||
| const oldUser = { ...this.user }; | ||
| this.user = { | ||
| ...this.user, | ||
| ...user, | ||
| }; | ||
|
|
||
| // Only notify if user properties actually changed | ||
| if (JSON.stringify(oldUser) !== JSON.stringify(this.user)) { | ||
| this.log("updateUserProperties()", this.user); | ||
| this.notifyListeners(); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Dynamically validating + updating user properties.
This method systematically checks each field against the specified validator. Excellent approach. If partial updates are frequent, consider a more robust pattern for ignoring undefined fields or merging them gracefully without re-validating unmodified properties.
| onClick={() => { | ||
| const newName = prompt("Enter new name", environment.name); | ||
| if (newName && newName !== environment.name) { | ||
| renameEnvironment({ | ||
| environmentId: environment.id, | ||
| name: newName, | ||
| }); | ||
| } | ||
| }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance the rename functionality.
The current implementation uses a basic prompt. Consider using a modal with proper validation and error handling.
Apply this diff to improve the rename functionality:
- const newName = prompt("Enter new name", environment.name);
- if (newName && newName !== environment.name) {
- renameEnvironment({
- environmentId: environment.id,
- name: newName,
- });
- }
+ setRenameState({
+ environmentId: environment.id,
+ currentName: environment.name,
+ });Add a new RenameEnvironmentModal component:
function RenameEnvironmentModal({
isOpen,
onClose,
environmentId,
currentName,
}: {
isOpen: boolean;
onClose: () => void;
environmentId: string;
currentName: string;
}) {
const [name, setName] = useState(currentName);
const [error, setError] = useState("");
const { mutate: renameEnvironment } = trpc.environments.updateName.useMutation({
onSuccess: () => {
toast.success("Environment renamed");
onClose();
},
onError: () => {
setError("Failed to rename environment");
},
});
return (
<Modal
title="Rename Environment"
isOpen={isOpen}
onClose={onClose}
onConfirm={() => {
if (!name.trim()) {
setError("Name is required");
return;
}
if (name === currentName) {
onClose();
return;
}
renameEnvironment({
environmentId,
name: name.trim(),
});
}}
>
<div className="space-y-4">
<div>
<Label>Environment Name</Label>
<Input
value={name}
onChange={(e) => {
setError("");
setName(e.target.value);
}}
placeholder="Enter environment name"
/>
{error && (
<p className="text-sm text-destructive mt-2">{error}</p>
)}
</div>
</div>
</Modal>
);
}| // biome-ignore lint/suspicious/noArrayIndexKey: <explanation> | ||
| key={index} |
There was a problem hiding this comment.
Avoid using array index as key.
Using array index as key in the key={index} prop can lead to rendering issues when items are reordered or deleted.
Consider using a unique identifier or generating a stable key:
-// biome-ignore lint/suspicious/noArrayIndexKey: <explanation>
-key={index}
+key={`rule-${index}-${JSON.stringify(rule)}`}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // biome-ignore lint/suspicious/noArrayIndexKey: <explanation> | |
| key={index} | |
| key={`rule-${index}-${JSON.stringify(rule)}`} |
| // biome-ignore lint/suspicious/noArrayIndexKey: <explanation> | ||
| key={index} | ||
| className="relative border-muted bg-muted/5" |
There was a problem hiding this comment.
Same issue with array index as key in rule rendering.
Using array index as key in rule rendering can lead to rendering issues.
Consider using a unique identifier or generating a stable key:
-// biome-ignore lint/suspicious/noArrayIndexKey: <explanation>
-key={index}
+key={`rule-card-${index}-${JSON.stringify(rule)}`}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // biome-ignore lint/suspicious/noArrayIndexKey: <explanation> | |
| key={index} | |
| className="relative border-muted bg-muted/5" | |
| key={`rule-card-${index}-${JSON.stringify(rule)}`} | |
| className="relative border-muted bg-muted/5" |
| // biome-ignore lint/suspicious/noArrayIndexKey: <explanation> | ||
| <div key={subRuleIndex} className="relative"> |
There was a problem hiding this comment.
Same issue with array index as key in nested mapping.
Using array index as key in nested mapping can also lead to rendering issues.
Consider using a unique identifier or generating a stable key:
-// biome-ignore lint/suspicious/noArrayIndexKey: <explanation>
-key={subRuleIndex}
+key={`subrule-${subRuleIndex}-${JSON.stringify(subRule)}`}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // biome-ignore lint/suspicious/noArrayIndexKey: <explanation> | |
| <div key={subRuleIndex} className="relative"> | |
| <div key={`subrule-${subRuleIndex}-${JSON.stringify(subRule)}`} className="relative"> |
| <p className="text-sm text-muted-foreground mt-1.5 line-clamp-1"> | ||
| {currentFlag.description.replace(/<[^>]*>/g, "")} | ||
| </p> |
There was a problem hiding this comment.
Improve HTML content sanitization.
Using regex to strip HTML tags is not a secure way to sanitize HTML content. Consider using a proper HTML sanitization library.
-{currentFlag.description.replace(/<[^>]*>/g, "")}
+import DOMPurify from 'dompurify';
+{DOMPurify.sanitize(currentFlag.description, { ALLOWED_TAGS: [] })}Committable suggestion skipped: line range outside the PR's diff.
| getFlagByValueId: protectedProcedure | ||
| .input( | ||
| z.object({ | ||
| flagValueId: z.string(), | ||
| }) | ||
| ) | ||
| .query(async ({ ctx, input }) => { | ||
| return ctx.prisma.featureFlagValue.findUnique({ | ||
| where: { | ||
| id: input.flagValueId, | ||
| }, | ||
| include: { | ||
| environment: true, | ||
| history: { include: { user: true } }, | ||
| flag: { | ||
| include: { | ||
| project: { | ||
| include: { | ||
| userSegments: true, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| ruleSets: true, | ||
| }, | ||
| }); | ||
| }), |
There was a problem hiding this comment.
Add authorization check to getFlagByValueId procedure.
The procedure retrieves sensitive flag data but lacks authorization validation. This could expose flag configurations to unauthorized users.
Add authorization check similar to other procedures:
getFlagByValueId: protectedProcedure
.input(
z.object({
flagValueId: z.string(),
})
)
.query(async ({ ctx, input }) => {
+ const flagValue = await ctx.prisma.featureFlagValue.findFirst({
+ where: {
+ id: input.flagValueId,
+ flag: {
+ project: {
+ users: {
+ some: {
+ userId: ctx.session.user.id,
+ },
+ },
+ },
+ },
+ },
+ });
+ if (!flagValue) throw new TRPCError({ code: "UNAUTHORIZED" });
+
return ctx.prisma.featureFlagValue.findUnique({
where: {
id: input.flagValueId,
},
include: {
environment: true,
history: { include: { user: true } },
flag: {
include: {
project: {
include: {
userSegments: true,
},
},
},
},
ruleSets: true,
},
});
}),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| getFlagByValueId: protectedProcedure | |
| .input( | |
| z.object({ | |
| flagValueId: z.string(), | |
| }) | |
| ) | |
| .query(async ({ ctx, input }) => { | |
| return ctx.prisma.featureFlagValue.findUnique({ | |
| where: { | |
| id: input.flagValueId, | |
| }, | |
| include: { | |
| environment: true, | |
| history: { include: { user: true } }, | |
| flag: { | |
| include: { | |
| project: { | |
| include: { | |
| userSegments: true, | |
| }, | |
| }, | |
| }, | |
| }, | |
| ruleSets: true, | |
| }, | |
| }); | |
| }), | |
| getFlagByValueId: protectedProcedure | |
| .input( | |
| z.object({ | |
| flagValueId: z.string(), | |
| }) | |
| ) | |
| .query(async ({ ctx, input }) => { | |
| const flagValue = await ctx.prisma.featureFlagValue.findFirst({ | |
| where: { | |
| id: input.flagValueId, | |
| flag: { | |
| project: { | |
| users: { | |
| some: { | |
| userId: ctx.session.user.id, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }); | |
| if (!flagValue) throw new TRPCError({ code: "UNAUTHORIZED" }); | |
| return ctx.prisma.featureFlagValue.findUnique({ | |
| where: { | |
| id: input.flagValueId, | |
| }, | |
| include: { | |
| environment: true, | |
| history: { include: { user: true } }, | |
| flag: { | |
| include: { | |
| project: { | |
| include: { | |
| userSegments: true, | |
| }, | |
| }, | |
| }, | |
| }, | |
| ruleSets: true, | |
| }, | |
| }); | |
| }), |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/node/tests/mocks/handlers.ts (1)
4-31: 💡 Verification agent🧩 Analysis chain
Verify the mock data structure aligns with the new user segments feature.
The
AbbyDataResponsetype should be updated to include user segments and conditional rules. Let's verify the type definition matches the new features.
🏁 Script executed:
#!/bin/bash # Description: Check if AbbyDataResponse type includes user segments and rules # Search for AbbyDataResponse type definition ast-grep --pattern 'type AbbyDataResponse = { $$$ }' # Search for any interfaces or types related to user segments and rules rg -t typescript '(type|interface).*(Rule|Segment|Condition)'Length of output: 1038
ACTION REQUIRED: Include user segments in the AbbyDataResponse definition
- The current type (located at
packages/core/src/shared/types.ts) only definestests,flags, andremoteConfig. There is no property for user segments, which are required by the new user segments feature.- While both
flagsandremoteConfiginclude an optionalruleSet(addressing conditional rules to some extent), the absence of a dedicated user segments property means that the mock data inpackages/node/tests/mocks/handlers.tsis not fully aligned with the updated feature requirements.- Please update the
AbbyDataResponsetype to include a user segments property (e.g.,userSegments: Array<YourSegmentType>), and modify the corresponding mock data accordingly.
🧹 Nitpick comments (1)
packages/node/tests/mocks/handlers.ts (1)
20-24: Consider adding test cases for user segments and conditional rules.Since this PR introduces user segments and conditional rules for feature flags, consider enhancing the mock data to include test cases for these new features.
{ name: "flag2", value: false, + rules: [ + { + value: true, + conditions: [ + { property: "country", operator: "equals", value: "US" } + ] + } + ] },
… and enhance user validation
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (8)
apps/web/src/server/trpc/router/flags.ts (1)
525-587: Consider improving rule set naming and validation.A few suggestions for improvement:
- The hardcoded "Default" name for new rule sets could be made configurable or more descriptive.
- Consider adding validation to ensure rule set names are unique within a flag value's scope.
if (!input.ruleSetId) { + const ruleSetCount = await ctx.prisma.flagRuleSet.count({ + where: { flagValueId: flagValue.id } + }); + const ruleSetName = `Rule Set ${ruleSetCount + 1}`; await ctx.prisma.flagRuleSet.create({ data: { flagValueId: flagValue.id, rules: input.ruleSet, - name: "Default", + name: ruleSetName, }, }); }packages/node/CHANGELOG.md (2)
5-6: Major Changes Section Introduced
The addition of the "### Major Changes" header is clear. It might be helpful to consider enriching the details in this section (beyond the brief note) to provide users with more context regarding the introduction of rules and user segments.
7-7: Detail on New Functionality
The bullet "- add rules and user segments" is succinct and accurately reflects the change. Consider adding a bit more detail or examples if future reviewers/users might benefit from further context on how these new features are intended to work.packages/angular/CHANGELOG.md (1)
7-8: Documenting New Feature EnhancementsThe bullet point "- add rules and user segments" effectively highlights the new functionality. For consistency and clarity, consider using an imperative tone (e.g., "Add rules and user segments")—if that aligns with your changelog style.
packages/remix/CHANGELOG.md (1)
5-8: Enhance Major Changes Description:
The "### Major Changes" section now lists "- add rules and user segments". While this is succinct, providing a bit more context—such as a brief summary of the new features or guidance on how these additions impact usage—could improve clarity for consumers of the changelog.packages/core/src/validation/index.ts (2)
8-16: Consider using explicit return types instead of type assertions.The factory functions and optional helper could benefit from more precise typing.
Apply this diff to improve type safety:
-export const string = () => ({ type: "string" }) as StringValidatorType; -export const number = () => ({ type: "number" }) as NumberValidatorType; -export const boolean = () => ({ type: "boolean" }) as BooleanValidatorType; +export const string = (): StringValidatorType => ({ type: "string" }); +export const number = (): NumberValidatorType => ({ type: "number" }); +export const boolean = (): BooleanValidatorType => ({ type: "boolean" }); -export const optional = < - T extends StringValidatorType | NumberValidatorType | BooleanValidatorType, ->( - type: T -) => ({ ...type, optional: true }) as const; +export const optional = <T extends ValidatorType>( + type: T +): T & { optional: true } => ({ ...type, optional: true });
18-30: Consider simplifying the type inference logic.The nested conditional types could be simplified using a union type and mapped types.
Consider this alternative implementation:
type ValidatorTypeMap = { string: string; number: number; boolean: boolean; }; export type Infer<T extends ValidatorType> = T extends { type: keyof ValidatorTypeMap } ? T extends { optional: true } ? ValidatorTypeMap[T['type']] | null | undefined : ValidatorTypeMap[T['type']] : never;apps/web/src/api/routes/v2_project_data.ts (1)
15-91: Potential single-pass optimization when categorizing flags.
Currently, the code filters and maps theflagsarray twice—once for boolean flags and once for other flags. Consider a single-pass transformation to reduce iterations, though it's more of a micro-optimization than a mandatory change.- flags: flags - .filter(({ flag }) => flag.type === "BOOLEAN") - .map((flagValue) => { ... }), - remoteConfig: flags - .filter(({ flag }) => flag.type !== "BOOLEAN") - .map((flagValue) => { ... }), + const booleanFlags: typeof flags = []; + const nonBooleanFlags: typeof flags = []; + for (const flagValue of flags) { + if (flagValue.flag.type === "BOOLEAN") { + booleanFlags.push(flagValue); + } else { + nonBooleanFlags.push(flagValue); + } + } + flags: booleanFlags.map((flagValue) => { ... }), + remoteConfig: nonBooleanFlags.map((flagValue) => { ... }),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
apps/web/CHANGELOG.md(1 hunks)apps/web/package.json(1 hunks)apps/web/src/api/index.ts(2 hunks)apps/web/src/api/routes/v1_project_data.ts(1 hunks)apps/web/src/api/routes/v2_project_data.ts(1 hunks)apps/web/src/server/trpc/router/flags.ts(2 hunks)packages/angular/CHANGELOG.md(1 hunks)packages/angular/package.json(1 hunks)packages/cli/CHANGELOG.md(1 hunks)packages/cli/package.json(1 hunks)packages/core/CHANGELOG.md(1 hunks)packages/core/package.json(2 hunks)packages/core/src/shared/http.ts(1 hunks)packages/core/src/validation/index.ts(1 hunks)packages/devtools/CHANGELOG.md(1 hunks)packages/devtools/package.json(1 hunks)packages/next/CHANGELOG.md(1 hunks)packages/next/package.json(1 hunks)packages/node/CHANGELOG.md(1 hunks)packages/node/package.json(2 hunks)packages/react/CHANGELOG.md(1 hunks)packages/react/package.json(1 hunks)packages/remix/CHANGELOG.md(1 hunks)packages/remix/package.json(1 hunks)packages/svelte/CHANGELOG.md(1 hunks)packages/svelte/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (10)
- packages/react/package.json
- packages/next/package.json
- packages/svelte/package.json
- packages/svelte/CHANGELOG.md
- packages/cli/package.json
- packages/devtools/CHANGELOG.md
- packages/remix/package.json
- packages/cli/CHANGELOG.md
- packages/core/CHANGELOG.md
- packages/angular/package.json
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/node/package.json
- apps/web/package.json
- apps/web/src/api/routes/v1_project_data.ts
🔇 Additional comments (27)
packages/devtools/package.json (1)
3-3: Major Version Update: Ensure Changelog & Breaking Change Documentation
The version is bumped to "8.0.0", which signals a major release that could introduce breaking changes. Given the updates in the feature flag documentation (including new user segments and conditional rules), please confirm that all breaking changes are clearly documented in the changelog and release notes.apps/web/src/server/trpc/router/flags.ts (1)
489-524: LGTM! Authorization properly implemented.The procedure now includes proper authorization checks through the where clause, ensuring users can only access flags from projects they belong to.
packages/next/CHANGELOG.md (3)
3-4: New Changelog Section for Version 7.0.0The new section is introduced clearly with the version header for 7.0.0. This provides a good structure for segregating major changes from previous releases.
5-8: Clear Major Changes DescriptionThe "### Major Changes" block with the line "- add rules and user segments" succinctly conveys the primary enhancement introduced in this release. This aligns well with the PR objectives focused on enhancing the feature flags with user segments and conditional rules.
9-13: Correct Patch Changes and Dependency UpdateThe "### Patch Changes" section documents the updated dependency on
@tryabby/react@7.0.0appropriately. Ensure that this dependency update is consistently reflected across all affected packages for cohesive versioning.apps/web/CHANGELOG.md (1)
3-11: New Version Entry (0.2.40) is Properly AddedThe new version entry starting at line 3 is formatted consistently with previous entries. The "Patch Changes" section correctly lists the updated dependency versions:
@tryabby/devtoolsis now at 8.0.0@tryabby/coreis now at 7.0.0@tryabby/nextis now at 7.0.0This update appears to align with the dependency bumps reflected in the broader PR context. Ensure that these dependency updates correspond with any downstream changes, especially given the significant updates in feature flag user segments and conditional rules elsewhere in the codebase.
packages/node/CHANGELOG.md (3)
3-4: New Version Entry "7.0.0" Added
The new version header "## 7.0.0" correctly indicates a major release update. Ensure that this header is clearly aligned with all accompanying changes in functionality.
9-9: Patch Changes Section Presence
The "### Patch Changes" header is present and clearly marks the patch-level updates. This separation from major changes improves clarity.
11-12: Dependency Update Noted Correctly
The updated dependency entry noting "@tryabby/core@7.0.0" is correctly reflected here. Double-check that all dependent packages are updated in accordance with the major changes documented.packages/core/package.json (3)
3-3: Version Bump to 7.0.0
Updating the version from "6.0.0" to "7.0.0" is appropriate given the significant enhancements in this release. Ensure that this version bump aligns with dependency updates across all related packages.
23-27: New Export for Validation Module
The new export entry for"./validation"extends the module API nicely. Verify that the paths (./dist/validation/index.jsand./dist/validation/index.d.ts) are correctly generated during the build process and are included in the published package.
28-31: New Export for Schema Module
Adding the export for"./schema"to provide access to shared schema types is a good enhancement for modularity and user accessibility. Please confirm that the corresponding files (./dist/shared/schemas.jsand./dist/shared/schemas.d.ts) are indeed present in the build output.packages/angular/CHANGELOG.md (4)
3-4: New Version Entry Added for 4.0.0This section correctly introduces version 4.0.0. The header is clear and follows the established format seen in previous entries.
5-6: Major Changes Section AddedThe "### Major Changes" header is appropriately added under the new version and is consistent with the formatting used in earlier versions.
9-10: Patch Changes Section AddedThe "### Patch Changes" header is added neatly to separate dependency updates from major changes. Its placement maintains consistency with the overall structure.
11-13: Dependency Update Recorded CorrectlyThe updated dependency entry for
@tryabby/core@7.0.0is clearly documented under the patch changes. This update aligns with the corresponding dependency bumps in other packages.packages/remix/CHANGELOG.md (2)
3-4: Version Header Confirmation:
The header "## 3.0.0" correctly indicates a major release. This aligns with the introduction of breaking changes such as the addition of rules and user segments. Consider adding a reference to detailed migration or release notes if applicable.
9-13: Dependency Upgrade Verification:
The patch changes document the upgrade of "@tryabby/react" from version 6.0.0 to 7.0.0. Ensure that this dependency update has been tested for compatibility across all related packages and that any breaking changes in the dependency are clearly communicated.packages/react/CHANGELOG.md (3)
3-4: New Version Header Added: "7.0.0"
This new version header clearly signals a major release of the package. Please ensure that any associated breaking changes or migration guidelines are documented elsewhere in the release notes.
5-8: Major Changes Section: Addition of Rules and User Segments
The "Major Changes" section now specifies the addition of rules and user segments. The description is clear and concise. Verify that all new functionalities related to these changes are consistently documented in the supporting documentation.
9-13: Patch Changes Update: Dependency Version Bump
The patch changes now list an updated dependency to@tryabby/core@7.0.0. Please confirm that this version update is compatible with the rest of the repository and that any requisite tests have been executed.packages/core/src/validation/index.ts (1)
89-89: Refine type assignment forreturnObject.Instead of using a broad type assertion, consider using a more specific type.
apps/web/src/api/routes/v2_project_data.ts (2)
1-12: All imports appear valid and necessary.
The imports cleanly address various needs: request handling, type definitions, caching, and queue processing. No immediate issues spotted.
13-14: Header naming looks consistent.
Defining a custom cache header underX-Abby-Cachefollows standard naming conventions.apps/web/src/api/index.ts (2)
9-9: Importing v2 route is straightforward and correct.
No issues spotted in this import statement.
24-25: New v2 route successfully integrated.
The route declaration for/v2/datais well-structured, aligning with the newmakeV2ProjectDataRoutefunction.packages/core/src/shared/http.ts (1)
21-21:Details
❓ Verification inconclusive
Switch to v2 endpoint aligns with the new route.
This change is consistent with the introduction ofv2_project_dataroutes.Consider searching for any lingering references to the old
api/v1/datausage to ensure all consumer-side calls are moved over or remain intentionally referencing v1. Here’s a script to check:
🏁 Script executed:
#!/bin/bash rg "api/v1/data"Length of output: 1158
Switch to v2 endpoint is correctly applied in packages/core/src/shared/http.ts.
The change now aligns with the introduction ofv2_project_dataroutes. However, after running a search for"api/v1/data", we discovered that the following files still reference the v1 endpoint:
Documentation:
apps/docs/pages/reference/http.mdx(several GET paths referencing/api/v1/data)Test Mocks:
packages/next/tests/mocks/handlers.tspackages/react/tests/mocks/handlers.tspackages/remix/tests/mocks/handlers.tspackages/node/tests/mocks/handlers.tspackages/core/tests/mocks/handlers.tspackages/svelte/src/tests/mocks/handlers.tsPlease verify that these residual references in documentation and tests are intentional. If the consumer-side calls are indeed intended to migrate fully to v2, update these references accordingly; otherwise, document the rationale for retaining the v1 endpoints in these areas.
| const returnObject: { | ||
| [key in keyof T]: Infer<T[keyof T]>; | ||
| } = {} as any; |
There was a problem hiding this comment.
Avoid using type assertion with 'any'.
Using as any bypasses TypeScript's type checking and could lead to runtime errors.
Apply this diff to improve type safety:
- const returnObject: {
- [key in keyof T]: Infer<T[keyof T]>;
- } = {} as any;
+ const returnObject = {} as {
+ [key in keyof T]: Infer<T[key]>;
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const returnObject: { | |
| [key in keyof T]: Infer<T[keyof T]>; | |
| } = {} as any; | |
| const returnObject = {} as { | |
| [key in keyof T]: Infer<T[key]>; | |
| }; |
| export function validateProperty<T extends ValidatorType>( | ||
| property: T, | ||
| value: unknown | ||
| ): string | undefined { | ||
| if (value === undefined && "optional" in property) { | ||
| return; | ||
| } | ||
|
|
||
| if (property.type === "string" && typeof value !== "string") { | ||
| return `Expected string but got ${typeof value}`; | ||
| } | ||
|
|
||
| if (property.type === "number" && typeof value !== "number") { | ||
| return `Expected number but got ${typeof value}`; | ||
| } | ||
|
|
||
| if (property.type === "boolean" && typeof value !== "boolean") { | ||
| return `Expected boolean but got ${typeof value}`; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve property validation with null checks and early returns.
The validation function could be more robust and readable.
Apply this diff to improve the implementation:
export function validateProperty<T extends ValidatorType>(
property: T,
value: unknown
): string | undefined {
- if (value === undefined && "optional" in property) {
+ if ((value === undefined || value === null) && "optional" in property) {
return;
}
+ if (value === null) {
+ return `Expected ${property.type} but got null`;
+ }
+
if (property.type === "string" && typeof value !== "string") {
return `Expected string but got ${typeof value}`;
}
if (property.type === "number" && typeof value !== "number") {
return `Expected number but got ${typeof value}`;
}
if (property.type === "boolean" && typeof value !== "boolean") {
return `Expected boolean but got ${typeof value}`;
}
+
+ return undefined;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function validateProperty<T extends ValidatorType>( | |
| property: T, | |
| value: unknown | |
| ): string | undefined { | |
| if (value === undefined && "optional" in property) { | |
| return; | |
| } | |
| if (property.type === "string" && typeof value !== "string") { | |
| return `Expected string but got ${typeof value}`; | |
| } | |
| if (property.type === "number" && typeof value !== "number") { | |
| return `Expected number but got ${typeof value}`; | |
| } | |
| if (property.type === "boolean" && typeof value !== "boolean") { | |
| return `Expected boolean but got ${typeof value}`; | |
| } | |
| } | |
| export function validateProperty<T extends ValidatorType>( | |
| property: T, | |
| value: unknown | |
| ): string | undefined { | |
| if ((value === undefined || value === null) && "optional" in property) { | |
| return; | |
| } | |
| if (value === null) { | |
| return `Expected ${property.type} but got null`; | |
| } | |
| if (property.type === "string" && typeof value !== "string") { | |
| return `Expected string but got ${typeof value}`; | |
| } | |
| if (property.type === "number" && typeof value !== "number") { | |
| return `Expected number but got ${typeof value}`; | |
| } | |
| if (property.type === "boolean" && typeof value !== "boolean") { | |
| return `Expected boolean but got ${typeof value}`; | |
| } | |
| return undefined; | |
| } |
| export function validate<T extends Record<string, ValidatorType>>( | ||
| userValidator: T, | ||
| user: Record<string, unknown> | ||
| ): |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for user validator schema.
The function should validate the input schema before processing.
Add input validation at the beginning of the function:
export function validate<T extends Record<string, ValidatorType>>(
userValidator: T,
user: Record<string, unknown>
) {
+ if (!userValidator || typeof userValidator !== 'object') {
+ throw new Error('Invalid validator schema');
+ }
+
+ if (!user || typeof user !== 'object') {
+ throw new Error('Invalid user object');
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function validate<T extends Record<string, ValidatorType>>( | |
| userValidator: T, | |
| user: Record<string, unknown> | |
| ): | |
| export function validate<T extends Record<string, ValidatorType>>( | |
| userValidator: T, | |
| user: Record<string, unknown> | |
| ) { | |
| if (!userValidator || typeof userValidator !== 'object') { | |
| throw new Error('Invalid validator schema'); | |
| } | |
| if (!user || typeof user !== 'object') { | |
| throw new Error('Invalid user object'); | |
| } | |
| // ... rest of the function implementation | |
| } |
| export function makeV2ProjectDataRoute() { | ||
| const app = new Hono() | ||
| .get( | ||
| "/:projectId", | ||
| zValidator( | ||
| "query", | ||
| z.object({ | ||
| environment: z.string(), | ||
| }) | ||
| ), | ||
| timing(), | ||
| async (c) => { | ||
| const projectId = c.req.param("projectId"); | ||
| const { environment } = c.req.valid("query"); | ||
|
|
||
| const now = performance.now(); | ||
|
|
||
| try { | ||
| startTime(c, "getAbbyResponseWithCache"); | ||
| const response = await getAbbyResponseWithCache({ | ||
| projectId, | ||
| environment, | ||
| c, | ||
| }); | ||
| endTime(c, "getAbbyResponseWithCache"); | ||
|
|
||
| const duration = performance.now() - now; | ||
|
|
||
| afterDataRequestQueue.add("after-data-request", { | ||
| apiVersion: "V1", | ||
| functionDuration: duration, | ||
| projectId, | ||
| }); | ||
|
|
||
| return c.json(response); | ||
| } catch (e) { | ||
| console.error(e); | ||
| return c.json({ error: "Internal server error" }, { status: 500 }); | ||
| } | ||
| } | ||
| ) | ||
| .get( |
There was a problem hiding this comment.
Double-check apiVersion: "V1" in the new v2 route.
Within the new v2 endpoint, lines 121–125 set apiVersion: "V1". This may cause downstream confusion or skew analytics.
- afterDataRequestQueue.add("after-data-request", {
- apiVersion: "V1",
- functionDuration: duration,
- projectId,
- });
+ afterDataRequestQueue.add("after-data-request", {
+ apiVersion: "V2",
+ functionDuration: duration,
+ projectId,
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function makeV2ProjectDataRoute() { | |
| const app = new Hono() | |
| .get( | |
| "/:projectId", | |
| zValidator( | |
| "query", | |
| z.object({ | |
| environment: z.string(), | |
| }) | |
| ), | |
| timing(), | |
| async (c) => { | |
| const projectId = c.req.param("projectId"); | |
| const { environment } = c.req.valid("query"); | |
| const now = performance.now(); | |
| try { | |
| startTime(c, "getAbbyResponseWithCache"); | |
| const response = await getAbbyResponseWithCache({ | |
| projectId, | |
| environment, | |
| c, | |
| }); | |
| endTime(c, "getAbbyResponseWithCache"); | |
| const duration = performance.now() - now; | |
| afterDataRequestQueue.add("after-data-request", { | |
| apiVersion: "V1", | |
| functionDuration: duration, | |
| projectId, | |
| }); | |
| return c.json(response); | |
| } catch (e) { | |
| console.error(e); | |
| return c.json({ error: "Internal server error" }, { status: 500 }); | |
| } | |
| } | |
| ) | |
| .get( | |
| export function makeV2ProjectDataRoute() { | |
| const app = new Hono() | |
| .get( | |
| "/:projectId", | |
| zValidator( | |
| "query", | |
| z.object({ | |
| environment: z.string(), | |
| }) | |
| ), | |
| timing(), | |
| async (c) => { | |
| const projectId = c.req.param("projectId"); | |
| const { environment } = c.req.valid("query"); | |
| const now = performance.now(); | |
| try { | |
| startTime(c, "getAbbyResponseWithCache"); | |
| const response = await getAbbyResponseWithCache({ | |
| projectId, | |
| environment, | |
| c, | |
| }); | |
| endTime(c, "getAbbyResponseWithCache"); | |
| const duration = performance.now() - now; | |
| afterDataRequestQueue.add("after-data-request", { | |
| apiVersion: "V2", | |
| functionDuration: duration, | |
| projectId, | |
| }); | |
| return c.json(response); | |
| } catch (e) { | |
| console.error(e); | |
| return c.json({ error: "Internal server error" }, { status: 500 }); | |
| } | |
| } | |
| ) | |
| .get( |
| "/:projectId/script.js", | ||
| cors({ | ||
| origin: "*", | ||
| maxAge: 86400, | ||
| }), | ||
| zValidator( | ||
| "query", | ||
| z.object({ | ||
| environment: z.string(), | ||
| }) | ||
| ), | ||
| timing(), | ||
| async (c) => { | ||
| const projectId = c.req.param("projectId"); | ||
| const { environment } = c.req.valid("query"); | ||
|
|
||
| const now = performance.now(); | ||
|
|
||
| try { | ||
| startTime(c, "getAbbyResponseWithCache"); | ||
| const response = await getAbbyResponseWithCache({ | ||
| projectId, | ||
| environment, | ||
| c, | ||
| }); | ||
| endTime(c, "getAbbyResponseWithCache"); | ||
|
|
||
| const jsContent = `window.${ABBY_WINDOW_KEY} = ${JSON.stringify( | ||
| response | ||
| )}`; | ||
|
|
||
| const duration = performance.now() - now; | ||
|
|
||
| afterDataRequestQueue.add("after-data-request", { | ||
| apiVersion: "V1", | ||
| functionDuration: duration, | ||
| projectId, | ||
| }); | ||
|
|
||
| return c.text(jsContent, { | ||
| headers: { | ||
| "Content-Type": "application/javascript", | ||
| }, | ||
| }); | ||
| } catch (e) { | ||
| console.error(e); | ||
| return c.json({ error: "Internal server error" }, { status: 500 }); | ||
| } | ||
| } | ||
| ); | ||
| return app; |
There was a problem hiding this comment.
Likewise correct apiVersion: "V1" in the JS route.
Repeating the above note for the second route: it also references apiVersion: "V1" (lines 168–172). Keep it consistent with the V2 naming.
- afterDataRequestQueue.add("after-data-request", {
- apiVersion: "V1",
- functionDuration: duration,
- projectId,
- });
+ afterDataRequestQueue.add("after-data-request", {
+ apiVersion: "V2",
+ functionDuration: duration,
+ projectId,
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "/:projectId/script.js", | |
| cors({ | |
| origin: "*", | |
| maxAge: 86400, | |
| }), | |
| zValidator( | |
| "query", | |
| z.object({ | |
| environment: z.string(), | |
| }) | |
| ), | |
| timing(), | |
| async (c) => { | |
| const projectId = c.req.param("projectId"); | |
| const { environment } = c.req.valid("query"); | |
| const now = performance.now(); | |
| try { | |
| startTime(c, "getAbbyResponseWithCache"); | |
| const response = await getAbbyResponseWithCache({ | |
| projectId, | |
| environment, | |
| c, | |
| }); | |
| endTime(c, "getAbbyResponseWithCache"); | |
| const jsContent = `window.${ABBY_WINDOW_KEY} = ${JSON.stringify( | |
| response | |
| )}`; | |
| const duration = performance.now() - now; | |
| afterDataRequestQueue.add("after-data-request", { | |
| apiVersion: "V1", | |
| functionDuration: duration, | |
| projectId, | |
| }); | |
| return c.text(jsContent, { | |
| headers: { | |
| "Content-Type": "application/javascript", | |
| }, | |
| }); | |
| } catch (e) { | |
| console.error(e); | |
| return c.json({ error: "Internal server error" }, { status: 500 }); | |
| } | |
| } | |
| ); | |
| return app; | |
| "/:projectId/script.js", | |
| cors({ | |
| origin: "*", | |
| maxAge: 86400, | |
| }), | |
| zValidator( | |
| "query", | |
| z.object({ | |
| environment: z.string(), | |
| }) | |
| ), | |
| timing(), | |
| async (c) => { | |
| const projectId = c.req.param("projectId"); | |
| const { environment } = c.req.valid("query"); | |
| const now = performance.now(); | |
| try { | |
| startTime(c, "getAbbyResponseWithCache"); | |
| const response = await getAbbyResponseWithCache({ | |
| projectId, | |
| environment, | |
| c, | |
| }); | |
| endTime(c, "getAbbyResponseWithCache"); | |
| const jsContent = `window.${ABBY_WINDOW_KEY} = ${JSON.stringify( | |
| response | |
| )}`; | |
| const duration = performance.now() - now; | |
| afterDataRequestQueue.add("after-data-request", { | |
| apiVersion: "V2", | |
| functionDuration: duration, | |
| projectId, | |
| }); | |
| return c.text(jsContent, { | |
| headers: { | |
| "Content-Type": "application/javascript", | |
| }, | |
| }); | |
| } catch (e) { | |
| console.error(e); | |
| return c.json({ error: "Internal server error" }, { status: 500 }); | |
| } | |
| } | |
| ); | |
| return app; |

This pull request introduces significant updates to the feature flag documentation, focusing on user segments and conditional rules. The changes include adding new sections to explain user properties, conditional rules, and their usage in various frameworks.
Documentation Enhancements:
User Segments & Conditional Rules:
Framework-Specific Updates:
userproperty configuration to thecreateAbbyfunction, allowing for user properties to be used in conditional rules.updateUserPropertiesfunction, which updates user properties for evaluating conditional rules, including validation and error handling.useFeatureFlagwith user properties for conditional rules. [1] [2] [3] [4] [5]userproperty configuration and detailed theupdateUserPropertiesfunction for user properties and conditional rules. [1] [2]Metadata Updates:
_meta.jsonfile to include the new documentation section in the navigation._meta.jsonfile to include the new rule operators page in the navigation.This pull request includes several updates to the documentation, primarily focusing on the addition of user segments, conditional rules, and operators for feature flags. These changes enhance the flexibility and targeting capabilities of feature flags based on user properties. Below are the most significant changes:Documentation Enhancements
feature-flags.mdx, explaining how to define user properties and create conditional rules for feature flags.userconfiguration option innextjs.mdx,react.mdx, andremix.mdxfiles, allowing for optional user properties that can be used to target feature flags. [1] [2] [3]updateUserPropertiesfunction innextjs.mdx,react.mdx, andremix.mdx, including validation and usage examples. [1] [2] [3]New Documentation Pages
operators.mdxdetailing the various operators available for creating conditional rules, including examples for string, number, and boolean operators.Metadata Updates
_meta.jsonfiles to include new entries for "User Segments" and "Operators" in the documentation. [1] [2]These updates significantly improve the documentation by providing comprehensive guides on using user properties and conditional rules to control feature flags, along with new metadata entries to better organize the content.
Summary by CodeRabbit
New Features
updateUserPropertiesfunction.Documentation
UI Improvements
Tests
Chores