Flags server sdk and advance feats#205
Conversation
|
|
|
@Sahil-Gupta584 is attempting to deploy a commit to the Databuddy OSS Team on Vercel. A member of the Team first needs to authorize it. |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds multivariant flags, flag dependencies, per-environment evaluation, scheduled rollouts with QStash integration, a server-side FlagsManager and Node SDK APIs, DB schema/table additions for schedules, shared flag schemas/utils, dashboard UI for variants/schedules/examples, RPC routes for flag schedules, webhooks, and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Scheduler as Scheduler service
participant DB as Database
participant API as API / Flag service
participant Cache as Cache layer
participant Dependents as Dependent flags
Scheduler->>DB: Query due schedules
DB-->>Scheduler: Return schedules
loop each executable schedule
Scheduler->>DB: Fetch flag by flagId
alt flag exists
Scheduler->>API: Compute updates (enable/disable/rollout step)
API->>DB: Persist flag update
DB-->>API: Confirm update
API->>Dependents: Query dependent flags in scope
Dependents-->>API: Return dependents
alt updated flag inactive
API->>DB: Mark active dependents inactive
else updated flag active
API->>DB: Activate dependents when prerequisites satisfied
end
API->>Cache: Invalidate affected cache entries
Scheduler->>DB: Mark schedule executed / update rolloutSteps
else flag missing
Scheduler->>DB: Delete orphan schedule
end
end
Scheduler->>Scheduler: Log summary
sequenceDiagram
participant Client as Client
participant PublicAPI as Public API
participant Cache as Flag Cache
participant Selector as selectVariant
participant Result as Evaluation
Client->>PublicAPI: evaluateFlag(key, context, environment)
PublicAPI->>Cache: getCachedFlag(key, clientId, environment)
Cache-->>PublicAPI: Return flag (may include variants)
alt multivariant flag
PublicAPI->>Selector: selectVariant(flag, context)
Selector->>Selector: Compute sticky or weighted assignment
Selector-->>PublicAPI: { value, variantKey }
PublicAPI->>Result: Compose FlagResult (enabled, value, variant, payload, reason)
else boolean/rollout
PublicAPI->>Result: Compose FlagResult (boolean/rollout outcome)
end
Result-->>Client: Return FlagResult
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 63
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/dashboard/app/providers.tsx (1)
80-104: Remove unusedFlagsProviderWrappercomponent.The
FlagsProviderWrappercomponent is defined but never used. The mainProviderscomponent directly integratesFlagsProviderinstead.Apply this diff to remove the dead code:
-function FlagsProviderWrapper({ children }: { children: React.ReactNode }) { - const { data: session, isPending } = useQuery({ - queryKey: SESSION_QUERY_KEY, - queryFn: async () => { - const result = await authClient.getSession(); - return result.data; - }, - staleTime: 2 * 60 * 1000, // 2 minutes - gcTime: 5 * 60 * 1000, // 5 minutes - }); - - return ( - <FlagsProvider - clientId="3ed1fce1-5a56-4cb6-a977-66864f6d18e3" - isPending={isPending} - user={ - session?.user - ? { userId: session.user.id, email: session.user.email } - : undefined - } - > - {children} - </FlagsProvider> - ); -}Also remove the now-unused
useQueryimport:import { QueryClient, QueryClientProvider, - useQuery, } from "@tanstack/react-query";packages/sdk/src/core/flags/types.ts (1)
1-8: Missingvariantfield and type mismatch for multivariant flags.Two issues with the
FlagResultinterface:
- The summary indicates a
variant?: stringfield should be added, but it's missing from the interface.- The
value: booleantype is incompatible with multivariant flags, which return variant values (e.g., numbers, strings). The consumer code inget-examples-strategy.tsalready assumesresult.valuecan be a number.Consider updating the interface:
export interface FlagResult { enabled: boolean; - value: boolean; + value: boolean | string | number | Record<string, unknown>; payload: any; reason: string; flagId?: string; flagType?: "boolean" | "rollout" | "multivariant"; + variant?: string; }apps/dashboard/app/(main)/websites/[id]/flags/_components/types.ts (1)
1-24: Dashboard Flag type now aligns with shared multivariant modelWiring
Flag.typetoFlagTypeand addingvariants?: Variant[]anddependencies?: string[]brings the dashboardFlaginterface in line with the shared flag schema, and theUserRule.operatorunion matches the operators used in evaluation logic/tests.If you plan to surface non‑boolean default values for multivariant or future flag types in the UI, consider widening
defaultValuefrombooleanto a union/unknown that matches the shared schema to avoid needing casts.Also applies to: 26-36
apps/dashboard/app/(main)/websites/[id]/flags/_components/flag-sheet.tsx (1)
350-365: InconsistentTooltipProviderusage.The tooltip at lines 350-365 doesn't have a
TooltipProviderwrapper, while the one at lines 482-503 does. This inconsistency could cause issues if there's no parentTooltipProvider.Either wrap the entire form with a single
TooltipProvideror ensure allTooltipusages are consistent.Also applies to: 482-503
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (35)
.gitmodules(0 hunks)apps/api/src/index.ts(1 hunks)apps/api/src/routes/public/flags.test.ts(2 hunks)apps/api/src/routes/public/flags.ts(6 hunks)apps/api/src/services/flag-scheduler.ts(1 hunks)apps/better-admin(0 hunks)apps/dashboard/app/(main)/websites/[id]/flags/_components/dependency-selector.tsx(1 hunks)apps/dashboard/app/(main)/websites/[id]/flags/_components/flag-examples.tsx(1 hunks)apps/dashboard/app/(main)/websites/[id]/flags/_components/flag-row.tsx(2 hunks)apps/dashboard/app/(main)/websites/[id]/flags/_components/flag-sheet.tsx(6 hunks)apps/dashboard/app/(main)/websites/[id]/flags/_components/flags-list.tsx(2 hunks)apps/dashboard/app/(main)/websites/[id]/flags/_components/schedule-manager.tsx(1 hunks)apps/dashboard/app/(main)/websites/[id]/flags/_components/types.ts(2 hunks)apps/dashboard/app/(main)/websites/[id]/flags/_components/variant-editor.tsx(1 hunks)apps/dashboard/app/providers.tsx(1 hunks)apps/dashboard/components/providers/organizations-provider.tsx(2 hunks)apps/dashboard/components/ui/switch.tsx(1 hunks)apps/dashboard/lib/flags/get-examples-strategy.ts(1 hunks)apps/dashboard/lib/utils.ts(1 hunks)packages/db/src/drizzle/schema.ts(4 hunks)packages/rpc/src/root.ts(2 hunks)packages/rpc/src/routers/flag-schedules.ts(1 hunks)packages/rpc/src/routers/flags.ts(12 hunks)packages/sdk/build.config.ts(1 hunks)packages/sdk/package.json(1 hunks)packages/sdk/src/core/flags/types.ts(2 hunks)packages/sdk/src/node/flags/create-manager.ts(1 hunks)packages/sdk/src/node/flags/flags-manager.ts(1 hunks)packages/sdk/src/node/flags/index.ts(1 hunks)packages/sdk/src/node/index.ts(1 hunks)packages/shared/package.json(1 hunks)packages/shared/src/flags/index.ts(1 hunks)packages/shared/src/flags/utils.ts(1 hunks)test-flag-scheduling.sh(1 hunks)turbo.json(1 hunks)
💤 Files with no reviewable changes (2)
- .gitmodules
- apps/better-admin
🧰 Additional context used
🧬 Code graph analysis (16)
apps/dashboard/app/(main)/websites/[id]/flags/_components/flag-examples.tsx (1)
apps/dashboard/lib/flags/get-examples-strategy.ts (1)
getExamplesDisplayStrategy(21-73)
packages/rpc/src/root.ts (1)
packages/rpc/src/routers/flag-schedules.ts (1)
flagSchedulesRouter(9-129)
apps/api/src/routes/public/flags.test.ts (1)
apps/api/src/routes/public/flags.ts (2)
selectVariant(227-277)evaluateFlag(279-329)
apps/dashboard/app/(main)/websites/[id]/flags/_components/variant-editor.tsx (1)
packages/shared/src/flags/index.ts (1)
Variant(34-34)
apps/dashboard/components/providers/organizations-provider.tsx (1)
apps/dashboard/stores/jotai/organizationsAtoms.ts (4)
organizationsAtom(4-4)activeOrganizationAtom(5-5)isLoadingOrganizationsAtom(6-6)getOrganizationBySlugAtom(8-11)
packages/rpc/src/routers/flag-schedules.ts (3)
packages/rpc/src/orpc.ts (1)
protectedProcedure(34-61)packages/db/src/drizzle/schema.ts (2)
flagSchedules(711-735)flags(641-709)packages/shared/src/flags/index.ts (1)
flagScheduleSchema(96-166)
apps/api/src/routes/public/flags.ts (2)
packages/redis/cacheable.ts (1)
cacheable(64-126)packages/db/src/drizzle/schema.ts (1)
flags(641-709)
apps/dashboard/app/providers.tsx (1)
packages/auth/src/client/auth-client.ts (1)
authClient(24-42)
packages/sdk/src/node/flags/flags-manager.ts (1)
packages/sdk/src/core/flags/types.ts (5)
FlagsManager(60-69)FlagsConfig(10-29)FlagResult(1-8)FlagsManagerOptions(53-58)FlagState(31-35)
apps/dashboard/components/ui/switch.tsx (1)
apps/dashboard/lib/utils.ts (1)
cn(5-7)
packages/shared/src/flags/utils.ts (2)
packages/redis/drizzle-cache.ts (1)
createDrizzleCache(31-249)packages/db/src/drizzle/schema.ts (1)
flags(641-709)
apps/dashboard/app/(main)/websites/[id]/flags/_components/schedule-manager.tsx (3)
packages/shared/src/flags/index.ts (1)
FlagType(36-36)packages/db/src/drizzle/schema.ts (1)
flagType(620-620)apps/dashboard/lib/utils.ts (2)
cn(5-7)formatDate(53-79)
apps/dashboard/lib/utils.ts (1)
apps/dashboard/lib/formatters.ts (1)
formatDate(64-83)
apps/dashboard/app/(main)/websites/[id]/flags/_components/types.ts (1)
packages/shared/src/flags/index.ts (2)
FlagType(36-36)Variant(34-34)
apps/dashboard/lib/flags/get-examples-strategy.ts (2)
packages/sdk/src/node/flags/create-manager.ts (1)
createServerFlagsManager(19-23)packages/sdk/src/node/flags/index.ts (1)
createServerFlagsManager(2-2)
packages/rpc/src/routers/flags.ts (3)
packages/shared/src/flags/index.ts (2)
flagFormSchema(37-76)variantSchema(24-33)packages/db/src/drizzle/schema.ts (1)
flags(641-709)packages/shared/src/flags/utils.ts (2)
getScopeCondition(21-36)handleFlagUpdateDependencyCascading(50-156)
🪛 Shellcheck (0.11.0)
test-flag-scheduling.sh
[info] 44-44: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 62-62: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 109-109: Double quote to prevent globbing and word splitting.
(SC2086)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Vercel Agent Review
🔇 Additional comments (28)
packages/sdk/package.json (1)
16-16: Verify the necessity and placement of thepinodependency in the client SDK.Adding Pino (a Node.js logging library) as a direct runtime dependency in a client SDK will unnecessarily bloat browser bundles (react, vue exports) and may cause bundler issues. The PR context mentions server-side flag features (scheduling, variants, dependencies), which typically belong in API/RPC packages, not the client SDK.
Please clarify:
- Why is
pinoneeded in the SDK package? If logging is required only for server-side flag management, it should reside in the API or RPC packages, not the SDK.- Is this logging only for the
./nodeexport? If so, consider making it an optional peer dependency or conditional dependency rather than a direct runtime dependency to avoid bloating browser-targeted exports.- Where and how is
pinoused in the SDK code? Please provide examples to confirm the necessity and correct placement.apps/dashboard/components/providers/organizations-provider.tsx (1)
70-73: LGTM!The deprecation notice is clear and provides a migration path for consumers.
apps/dashboard/lib/utils.ts (1)
53-79: Duplicate date formatting function with inconsistent behavior.A
formatDatefunction already exists informatters.ts(lines 63-82) with different behavior:
- Invalid date handling: Returns
'Invalid Date'here vs. empty string""in the existing function- Library choice: Uses native
Intl.DateTimeFormathere vs.dayjsin the existing function- Format flexibility: Supports only 2 hardcoded formats here vs. any dayjs format string in the existing function
Additionally, the
formatTypeparameter value'PPP p'suggests date-fns format tokens, but the implementation doesn't actually use date-fns—this is misleading.Consider consolidating these functions into a single implementation to avoid maintenance burden and inconsistency across the codebase.
apps/dashboard/components/ui/switch.tsx (1)
15-17: LGTM! Good styling enhancements.The addition of
shadow-xsandcursor-pointerimproves the visual appearance and provides better cursor feedback for users.apps/dashboard/app/providers.tsx (1)
48-48: Confirm the blank-screen UX during auth initialization.Returning
nullwhenisPending && !sessionblocks the entire app from rendering during the initial auth check. Users will see a blank screen until session data loads. Consider showing a loading spinner or skeleton UI instead.Is this blank-screen behavior intentional for security, or should a loading state be displayed?
packages/sdk/build.config.ts (1)
13-13: LGTM!The new build entry for the server-side flags module follows the existing pattern and correctly exposes the new
createServerFlagsManagerAPI as part of the SDK's public surface.turbo.json (1)
3-3: Verify if this UI mode change is intentional.Changing from
"tui"to"stream"disables the interactive terminal UI. This is typically preferred for CI/CD pipelines but may degrade local developer experience. Confirm this is an intentional project-wide change rather than a local preference that was accidentally committed.packages/sdk/src/core/flags/types.ts (1)
28-28: LGTM!The
environmentfield addition enables environment-scoped flag evaluation, aligning with the broader PR changes.apps/dashboard/lib/flags/get-examples-strategy.ts (1)
39-60: LGTM on the flag evaluation logic.The try/catch structure with graceful fallback is appropriate for server-side flag evaluation. The variant extraction and type coercion for
exampleCounthandles the multivariant payload correctly.apps/dashboard/app/(main)/websites/[id]/flags/_components/schedule-manager.tsx (1)
41-54: LGTM on the overall component structure.The header toggle and schedule type selection UI are well-organized. The pattern of watching form values and conditionally rendering schedule details is appropriate for this use case.
Also applies to: 56-91
packages/db/src/drizzle/schema.ts (2)
615-631: LGTM on new enums.The
dbPermissionLevel, extendedflagType, andflagScheduleActionTypeenums are well-defined. Minor note: the variable nameflagScheduleActionTypediffers from the DB enum name"flag_schedule_type"- consider aligning them for consistency, but this is not blocking.
659-661: LGTM on new flag columns.The
variants,dependencies, andenvironmentcolumns are well-structured for supporting multivariant flags with dependency management and environment scoping.packages/shared/src/flags/index.ts (1)
62-76: LGTM on multivariant weight validation.The validation correctly requires weights to sum to 100% only when weights are explicitly specified, allowing flexibility for equal distribution when weights are omitted.
packages/sdk/src/node/flags/flags-manager.ts (1)
18-56: LGTM on initialization pattern.The async initialization with
waitForInitialization()is a good pattern that allows callers to await readiness when needed while not blocking construction.apps/api/src/index.ts (1)
28-32: VerifystartFlagSchedulerinitialization and error handling.
startFlagScheduler()is called during server initialization without error handling. If this function throws or returns a rejected promise, the server startup will fail silently or crash.Consider wrapping the call or ensuring the scheduler handles its own errors internally.
packages/sdk/src/node/index.ts (1)
538-538: Node SDK: new flags barrel export looks safeThe
export * from "./flags";addition cleanly exposes the new server flags APIs from the main node entrypoint without conflicting with existing exports. This is a backward‑compatible surface expansion.packages/rpc/src/root.ts (1)
8-8: RPC: flagSchedules router wiring is consistentAdding
flagSchedulesRoutertoappRouterfollows the existing pattern for other routers and cleanly exposes the new scheduling endpoints viaappRouter.flagSchedules.Also applies to: 27-27
packages/sdk/src/node/flags/index.ts (1)
1-2: Flags SDK barrel: exports are coherent and minimalThe barrel re-export of
ServerFlagsManagerplus the two factory helpers is straightforward and matches whatpackages/sdk/src/node/index.tsnow exposes viaexport * from "./flags";. No issues from an API‑surface standpoint.packages/sdk/src/node/flags/create-manager.ts (1)
1-42: ServerFlagsManager factories are straightforward and backward‑compatibleThe
createServerFlagsManagerandcreateServerFlagsManagerInMemoryhelpers correctly wrapServerFlagsManagerconstruction and give you a stable public creation API, while preserving the legacyInMemoryname. The JSDoc examples match the@databuddy/sdk/nodere-export path.packages/shared/package.json (1)
31-32: Verify flags entrypoints exist and are included in build pipelineThe new exports for
"./flags"and"./flags/utils"and the@databuddy/redisworkspace dependency have been added to the package.json. Ensure thatpackages/shared/src/flags/index.tsandpackages/shared/src/flags/utils.tsexist and are included in your build/publish pipeline, and that the@databuddy/redisworkspace package is correctly configured.apps/api/src/routes/public/flags.test.ts (1)
1-10: Multivariant and selectVariant tests give good coverageThe new
selectVariantand multivariantevaluateFlagtests exercise:
- Deterministic, sticky assignment based on user hash,
- Support for string/number/object variant values,
- Fallback when no variants exist, and
- Proper wiring into
evaluateFlagincludingvariant,value,enabled,payload, and the"VARIANT_SELECTED"reason.This is solid behavioral coverage for the new multivariant path. Just keep the
"VARIANT_SELECTED"reason string in sync with any shared enums/constants used by clients to avoid drift.apps/dashboard/app/(main)/websites/[id]/flags/_components/flag-examples.tsx (1)
32-213: FLAG_TEMPLATES is well-structured with practical examples.The template definitions provide good coverage of multivariant use cases. The variant weights sum to 100% in all templates, and the code examples are instructive.
apps/dashboard/app/(main)/websites/[id]/flags/_components/flags-list.tsx (1)
52-68: Filtering logic is clean and correct.The combined status and search filtering is well-structured with proper null-safe access using optional chaining.
apps/dashboard/app/(main)/websites/[id]/flags/_components/variant-editor.tsx (2)
162-283: Well-structured variant editing UI.The per-variant editing interface with inline type selection, weight sliders, and description fields provides good UX. The visual feedback for weight validation (color-coded status) is helpful.
120-127: Verify backend weight constraint enforcement with backend team.Ensure the backend's
selectVariantlogic and weight validation properly handles both undefined weights (Even Distribution) and weighted variants. If the backend requires weights to sum to 100 when defined, this validation should be documented and enforced consistently across all entry points.apps/api/src/routes/public/flags.ts (1)
261-276: Weighted selection fallback may produce unexpected results.When weights don't sum to 100 (e.g., weights are [30, 30, 30] = 90%), users with percentages 90-99 fall through to the last variant. This creates uneven distribution where the last variant gets extra traffic.
Consider either:
- Normalizing weights at runtime:
cumulative += (variant.weight / totalWeight) * 100- Using
defaultValueas fallback instead of last variant- Documenting this behavior clearly
Verify that
flagFormSchemavalidation (which requires weights sum to 100) is applied before data reaches this evaluation code.packages/rpc/src/routers/flags.ts (2)
283-291: Circular dependency detection is shallow.The check only detects direct circular dependencies (A ↔ B). Transitive cycles like A → B → C → A would not be caught.
Consider whether deeper cycle detection is needed for your use case. If flags can have long dependency chains, you may want to implement a full graph traversal:
async function detectCycle(flagKey: string, dependencies: string[], visited = new Set<string>()): Promise<boolean> { if (visited.has(flagKey)) return true; visited.add(flagKey); // ... recursively check each dependency's dependencies }
463-465: LGTM: Cascading dependency updates are properly handled.The integration with
handleFlagUpdateDependencyCascadingensures dependent flags are updated when a flag's status changes. This maintains consistency across the dependency graph.
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
|
Refining more, will notify you for review |
There was a problem hiding this comment.
Actionable comments posted: 25
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
apps/dashboard/app/providers.tsx (2)
80-104: Remove unusedFlagsProviderWrappercomponent.
FlagsProviderWrapperis defined but never used—the mainProviderscomponent rendersFlagsProviderdirectly (lines 60-71). This dead code also contains another hardcodedclientIdthat would need the same env-var fix.-// Query key for session - shared with other components for deduplication -export const SESSION_QUERY_KEY = ["auth", "session"] as const; - -function FlagsProviderWrapper({ children }: { children: React.ReactNode }) { - const { data: session, isPending } = useQuery({ - queryKey: SESSION_QUERY_KEY, - queryFn: async () => { - const result = await authClient.getSession(); - return result.data; - }, - staleTime: 2 * 60 * 1000, // 2 minutes - gcTime: 5 * 60 * 1000, // 5 minutes - }); - - return ( - <FlagsProvider - clientId="3ed1fce1-5a56-4cb6-a977-66864f6d18e3" - isPending={isPending} - user={ - session?.user - ? { userId: session.user.id, email: session.user.email } - : undefined - } - > - {children} - </FlagsProvider> - ); -}If
SESSION_QUERY_KEYis used elsewhere, keep only that export.
8-8: Unused import:useQuery.
useQueryis imported but only used within the deadFlagsProviderWrappercomponent. Remove it along with the unused component.import { QueryClient, QueryClientProvider, - useQuery, } from "@tanstack/react-query";packages/sdk/src/core/flags/types.ts (1)
1-8: Type inconsistency:valueproperty may not always be boolean for multivariant flags.The
FlagResult.valueis typed asboolean(line 3), but multivariant flags can return values of any type (string, number, object) based on theselectVariantimplementation inapps/api/src/routes/public/flags.ts. This type mismatch could cause runtime issues or require unsafe type assertions.export interface FlagResult { enabled: boolean; - value: boolean; + value: boolean | string | number | Record<string, unknown>; payload: any; reason: string; flagId?: string; flagType?: "boolean" | "rollout" | "multivariant"; + variant?: string; }Note: The
variantproperty is also returned byevaluateFlagfor multivariant flags but is missing from this interface.packages/db/src/drizzle/schema.ts (1)
640-675: Environment column is not part of uniqueness, making per-environment flags impossibleYou’ve added
environmenttoflags, and the evaluation code now filters byflags.environment. However, the unique indexes still enforce uniqueness only on(key, websiteId),(key, organizationId), and(key, userId).This means you cannot store multiple flags with the same
keyandwebsiteIdfor different environments (e.g.stagingvsproduction) without hitting a uniqueness violation. If the intent is true environment scoping, these unique indexes should likely includeenvironment(and you may also want supporting indexes that include it for lookup performance).Please confirm the intended model and, if environment-specific flags are expected, adjust the unique indexes (and corresponding migrations) to include
environment.apps/api/src/routes/public/flags.ts (1)
40-46:FlagResult.valueis typed as boolean but now returns arbitrary variant values
FlagResultis defined as:type FlagResult = { enabled: boolean; value: boolean; payload: unknown; reason: string; variant?: string; };However, for multivariant flags you now set:
const { value, variant } = selectVariant(flag, context); // … value, // may be string/number/object variant,So at runtime
valuecan be non-boolean (string/number/JSON), while the type still claims it’sboolean. Becausevalueis coming fromany, TypeScript won’t catch this, but it’s an inaccurate public contract and can break callers that assume a boolean.Consider broadening the type of
valueto reflect reality, e.g.:type FlagResult = { enabled: boolean; value: boolean | string | number | Record<string, unknown> | null; payload: unknown; reason: string; variant?: string; };(or a more precise union that matches your variant schema), and update any Elysia typings accordingly.
Also applies to: 226-259, 275-283
packages/rpc/src/routers/flags.ts (1)
124-138:passthrough()allows arbitrary fields to be written to the database.The
updateFlagSchemauses.passthrough()(line 138), allowing unknown properties through validation. Combined with the spread at line 455 (...updates), any extra field from the client could be written to the database if a matching column exists. This is a security and data integrity concern.Consider using
.strict()instead, or explicitly pick only the fields you want to update.const updateFlagSchema = z.object({ id: z.string(), name: z.string().min(1).max(100).optional(), description: z.string().optional(), type: z.enum(["boolean", "rollout", "multivariant"]).optional(), status: z.enum(["active", "inactive", "archived"]).optional(), defaultValue: z.boolean().optional(), payload: z.any().optional(), rules: z.array(userRuleSchema).optional(), persistAcrossAuth: z.boolean().optional(), rolloutPercentage: z.number().min(0).max(100).optional(), variants: z.array(variantSchema).optional(), dependencies: z.array(z.string()).optional(), forceCancelScheduledRollout: z.boolean().optional(), -}).passthrough(); + environment: z.string().optional(), +});
♻️ Duplicate comments (30)
apps/dashboard/components/providers/organizations-provider.tsx (1)
78-78: Remove unused code (previously flagged).Line 78 declares
getOrganizationBySlugbut it's no longer used after switching to the inlineorganizations.find()approach on lines 84-85. The import ofgetOrganizationBySlugAtomon line 9 is also unused.Apply this diff to remove the unused code:
import { activeOrganizationAtom, - getOrganizationBySlugAtom, isLoadingOrganizationsAtom, organizationsAtom, } from "@/stores/jotai/organizationsAtoms";export function useOrganizationsContext() { const organizations = useAtomValue(organizationsAtom); const activeOrganization = useAtomValue(activeOrganizationAtom); const isLoading = useAtomValue(isLoadingOrganizationsAtom); - const [getOrganizationBySlug] = useAtom(getOrganizationBySlugAtom); return {apps/dashboard/components/ui/switch.tsx (1)
3-3: Fix Radix Switch import path and namespace usage.The current import from
"radix-ui"will fail module resolution, and importingSwitch as SwitchPrimitivedoes not match how you later useSwitchPrimitive.RootandSwitchPrimitive.Thumb. Radix’s Switch should be imported as a namespace from@radix-ui/react-switch.Apply this diff to fix the import:
-import { Switch as SwitchPrimitive } from "radix-ui"; +import * as SwitchPrimitive from "@radix-ui/react-switch";Please also verify that
@radix-ui/react-switchis installed and that this path matches the Radix UI version used in the project.apps/dashboard/app/providers.tsx (2)
43-45: Session state synchronization issue.This concern was already raised in a previous review.
61-61: HardcodedclientIdshould use environment variable.This concern was already raised in a previous review.
packages/shared/src/flags/utils.ts (5)
9-19: ConsiderPromise.allSettledfor cache invalidation.This concern was already raised in a previous review.
21-36: Fallback behavior returns a "match nothing" condition.This concern was already raised in a previous review.
95-124: N+1 query pattern in dependency resolution.This concern was already raised in a previous review.
127-153: Missing recursive cascading for transitive dependencies.This concern was already raised in a previous review.
113-115: Incomplete dependency validation when dependencies are missing.If a dependency key references a deleted or non-existent flag,
depFlagDependencieswill have fewer entries thandeps. The current checkdepFlagDependencies.every(...)would incorrectly returntruefor an empty array or when missing deps aren't accounted for.Add a length check to ensure all dependencies were found:
+ if (depFlagDependencies.length !== deps.length) { + // Some dependencies don't exist - skip activation + continue; + } + const allDependenciesActive = depFlagDependencies.every( (df) => df.status === "active" );apps/dashboard/app/(main)/websites/[id]/flags/_components/flag-row.tsx (1)
80-84: Keyboard double-toggle issue remains unaddressed.The
onKeyDownhandler fires for key events from any child element. Pressing Enter/Space on the inner toggle button will trigger both the Card'sonKeyDownand the Button'sonClick, causing a double-toggle. This issue was previously flagged.packages/sdk/src/node/flags/flags-manager.ts (5)
112-118: Incomplete user comparison may return stale results.The
isDifferentUsercheck only comparesuserId, ignoringproperties. This issue was previously flagged and remains unaddressed.
156-157: Remove debugconsole.logstatements.These
console.logstatements appear to be leftover debug code. This issue was previously flagged and remains unaddressed.
167-167: Remove debugconsole.logstatement.This
console.logstatement should be removed or converted tologger.debug.
218-219: Unhandled promise rejection.The
getFlag(key)call is intentionally not awaited, but if it rejects, this will cause an unhandled promise rejection. This issue was previously flagged and remains unaddressed.
238-251: Unhandled promises inupdateUserandupdateConfig.Both methods trigger async operations (
refresh()andfetchAllFlags()) without awaiting or handling rejections. This issue was previously flagged and remains unaddressed.apps/dashboard/app/(main)/websites/[id]/flags/_components/flag-examples.tsx (2)
215-220: Unused props in interface.
maxExamples,variant, andshowExamplesare defined inFlagExamplesPropsbut never used in the component.
229-232: Remove debug console.log statement.Debug logging should be removed before merging.
apps/dashboard/lib/flags/get-examples-strategy.ts (2)
41-41: Remove or conditionalize debug logging.The
console.logstatement runs unconditionally in production.
27-33: Consider caching the flags manager instance.Creating a new
ServerFlagsManagerand awaiting initialization on every request is inefficient.apps/api/src/routes/public/flags.ts (1)
40-46: Multivariant reason string doesn’t match tests/expected contractFor multivariant flags, you currently return:
return { enabled: true, value, variant, payload: flag.payload, reason: "MULTIVARIANT_EVALUATED", };Existing tests (and prior review feedback) expect the reason
"VARIANT_SELECTED", and this mismatch will cause test failures and potentially confuse downstream consumers that inspectreason.Recommend changing the reason to the agreed string:
- reason: "MULTIVARIANT_EVALUATED", + reason: "VARIANT_SELECTED",Also applies to: 275-283
apps/dashboard/app/(main)/websites/[id]/flags/_components/flags-list.tsx (3)
47-50: Hardcoded websiteId and unusedisFlagExamplesEnabledQueryThe
getShouldShowExamplesquery is currently:const isFlagExamplesEnabledQuery = useQuery({ queryKey: ["isFlagExamplesEnabled"], queryFn: () => getShouldShowExamples("OSA-FWQhcahi6J5VDxsbn", "", "production"), });Issues:
websiteIdis hardcoded instead of coming from the route ([id]) or actual client context.userIdandenvironmentare hardcoded/empty, which defeats the per-user and per-environment behavior described ingetShouldShowExamples.- The query result is never used, so the feature-flag check doesn’t actually gate anything; the “View/Hide Examples” button is always available.
Suggestion:
- Derive
websiteIdfromuseParams()(and, if available, inject realuserIdandenvironment).- Include those values in the
queryKeyto avoid cache collisions.- Use
isFlagExamplesEnabledQuery.data(and/or loading/error state) to decide whether to show or enable the examples button.
104-123: Duplicate funnel icon in status filterYou’re rendering
FunnelSimpleIconboth outside and inside theSelectTrigger, resulting in a duplicated icon in the UI. Keeping only the icon inside the trigger is sufficient and cleaner.You can safely remove the outer icon at Line 105.
141-147: Template data is ignored when creating from examplesThe
onCreateFromTemplatehandler ignores thetemplateargument and only toggles UI + callsonCreateFlagAction:onCreateFromTemplate={(template) => { // TODO: Auto-fill create form with template setShowExamples(false); onCreateFlagAction(); }}If the intent is to pre-fill the create form from the selected example, you’ll need to persist the template in state (or pass it through
onCreateFlagAction) and have the create form initialize from that data. As-is, selecting a template doesn’t influence the created flag.apps/dashboard/app/(main)/websites/[id]/flags/_components/schedule-manager.tsx (2)
67-89: FormField is not wired to the actual schedule fieldThe header toggle
FormFieldis declared as:<FormField // control={form.control} name="isEnabled" …while you actually store state under
schedule.isEnabledand watch it viaform.watch("schedule.isEnabled"). This means:
- RHF isn’t managing the correct field (and
controlis commented out).- Any validation/error display for scheduling won’t be associated with
schedule.isEnabled.You should bind this field to the real path and hook it into the form control:
- <FormField - // control={form.control} - name="isEnabled" + <FormField + control={form.control} + name="schedule.isEnabled"
96-126: Auto-adding rollout steps still uses the old type valueIn the schedule type
Select, the handler checkswatchedScheduledType(the previous value) instead of the newly selected one:<Select onValueChange={(e) => { if ( watchedScheduledType === "update_rollout" && (rolloutSteps.length === 0 || !rolloutSteps) ) { addRolloutStep(); } field.onChange(e); }} >This will not add a rollout step when the user switches to
"update_rollout", and can misfire when leaving that type.Use the new value instead:
- onValueChange={(e) => { - if (watchedScheduledType === "update_rollout" && (rolloutSteps.length === 0 || !rolloutSteps)) { - addRolloutStep(); - } - field.onChange(e); - }} + onValueChange={(e) => { + if (e === "update_rollout" && (!rolloutSteps || rolloutSteps.length === 0)) { + addRolloutStep(); + } + field.onChange(e); +}}apps/api/src/services/flag-scheduler.ts (2)
25-28: Use logger instead ofconsole.logfor scheduler diagnostics
processSchedulesstill usesconsole.log('Processing Schedules');while the rest of the module useslogger. For consistency and centralized logging, this should be:logger.debug("Processing schedules");(or
infoif you prefer).
88-161: Type-safety:schedandupdatesareany
executeSchedulecurrently usessched: anyandconst updates: any = {}. This loses type safety for both schedule metadata (e.g.,stepValue,stepScheduledAt) and flag updates.Consider defining an interface that describes the executable schedule shape (including the step augmentation) and typing
updatesasPartial<InferInsertModel<typeof flags>>(or similar). This will help catch typos on fields and make refactors safer.Not blocking, but worth tightening when you next touch this code.
packages/rpc/src/routers/flag-schedules.ts (1)
31-33: NOT_FOUND message should indicate missing schedule, not missing flagIn
getByFlagId, when no schedule exists you throw:if (!schedules[0]) { throw new ORPCError("NOT_FOUND", { message: "Flag not found" }); }At this point the flag has already been successfully loaded; the missing resource is the schedule. For clarity (and better client error handling), this should say
"Schedule not found".packages/shared/src/flags/index.ts (2)
24-34: Type mismatch:type: "json"butvaluedoesn't support objects.The
typeenum includes"json", but thevaluefield only acceptsstring | number. If JSON variants are intended, either extendvalueto support objects or document that JSON must be stored as a stringified value.
155-179: Missing date validity check forrolloutSteps[].scheduledAt.The validation checks if
scheduledAtis in the future (line 172) but doesn't first validate that it's a parseable date. Ifstep.scheduledAtis an invalid date string,new Date(step.scheduledAt).getTime()returnsNaN, and the comparisonDate.now() > NaNis alwaysfalse, silently passing invalid dates.for (const step of data.rolloutSteps) { + const stepDate = new Date(step.scheduledAt); + if (isNaN(stepDate.getTime())) { + ctx.addIssue({ + code: "custom", + path: ["rolloutSteps"], + message: "Invalid date in rollout step", + }); + continue; + } if (typeof step.value !== "number") { ctx.addIssue({ code: "custom", path: ["value"], message: "Step value must be a number between 0 and 100 for rollout steps", }); continue } if (step.value < 0 || step.value > 100) { ctx.addIssue({ code: "custom", path: ["value"], message: "Step value must be a number between 0 and 100 for rollout steps", }); } - if (Date.now() > new Date(step.scheduledAt!).getTime()) { + if (Date.now() > stepDate.getTime()) { ctx.addIssue({ code: "custom", path: ['rolloutSteps'], message: "Scheduled time must be in the future", }) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (34)
.gitmodules(0 hunks)apps/api/src/index.ts(1 hunks)apps/api/src/routes/public/flags.test.ts(2 hunks)apps/api/src/routes/public/flags.ts(6 hunks)apps/api/src/services/flag-scheduler.ts(1 hunks)apps/better-admin(0 hunks)apps/dashboard/app/(main)/websites/[id]/flags/_components/dependency-selector.tsx(1 hunks)apps/dashboard/app/(main)/websites/[id]/flags/_components/flag-examples.tsx(1 hunks)apps/dashboard/app/(main)/websites/[id]/flags/_components/flag-row.tsx(2 hunks)apps/dashboard/app/(main)/websites/[id]/flags/_components/flag-sheet.tsx(1 hunks)apps/dashboard/app/(main)/websites/[id]/flags/_components/flags-list.tsx(1 hunks)apps/dashboard/app/(main)/websites/[id]/flags/_components/schedule-manager.tsx(1 hunks)apps/dashboard/app/(main)/websites/[id]/flags/_components/types.ts(2 hunks)apps/dashboard/app/(main)/websites/[id]/flags/_components/variant-editor.tsx(1 hunks)apps/dashboard/app/providers.tsx(1 hunks)apps/dashboard/components/providers/organizations-provider.tsx(2 hunks)apps/dashboard/components/ui/switch.tsx(1 hunks)apps/dashboard/lib/flags/get-examples-strategy.ts(1 hunks)apps/dashboard/lib/utils.ts(1 hunks)packages/db/src/drizzle/schema.ts(3 hunks)packages/rpc/src/root.ts(2 hunks)packages/rpc/src/routers/flag-schedules.ts(1 hunks)packages/rpc/src/routers/flags.ts(12 hunks)packages/sdk/build.config.ts(1 hunks)packages/sdk/package.json(1 hunks)packages/sdk/src/core/flags/types.ts(2 hunks)packages/sdk/src/node/flags/create-manager.ts(1 hunks)packages/sdk/src/node/flags/flags-manager.ts(1 hunks)packages/sdk/src/node/flags/index.ts(1 hunks)packages/sdk/src/node/index.ts(1 hunks)packages/shared/package.json(1 hunks)packages/shared/src/flags/index.ts(1 hunks)packages/shared/src/flags/utils.ts(1 hunks)turbo.json(1 hunks)
💤 Files with no reviewable changes (2)
- .gitmodules
- apps/better-admin
🧰 Additional context used
🧬 Code graph analysis (18)
apps/api/src/index.ts (2)
packages/rpc/src/lib/otel.ts (1)
setupUncaughtErrorHandlers(120-128)apps/api/src/services/flag-scheduler.ts (1)
startFlagScheduler(10-15)
packages/rpc/src/root.ts (1)
packages/rpc/src/routers/flag-schedules.ts (1)
flagSchedulesRouter(9-143)
apps/dashboard/components/ui/switch.tsx (1)
apps/dashboard/lib/utils.ts (1)
cn(5-7)
apps/dashboard/app/providers.tsx (2)
packages/auth/src/client/auth-client.ts (1)
authClient(24-42)packages/db/src/drizzle/schema.ts (2)
session(104-136)user(314-338)
apps/dashboard/app/(main)/websites/[id]/flags/_components/flag-examples.tsx (1)
apps/dashboard/lib/flags/get-examples-strategy.ts (2)
getExamplesDisplayStrategy(21-66)getShouldShowExamples(68-78)
apps/dashboard/app/(main)/websites/[id]/flags/_components/schedule-manager.tsx (3)
apps/dashboard/components/ui/switch.tsx (1)
Switch(31-31)apps/api/src/query/expressions.ts (1)
field(414-418)apps/dashboard/lib/utils.ts (2)
cn(5-7)formatDate(53-79)
apps/api/src/routes/public/flags.test.ts (1)
apps/api/src/routes/public/flags.ts (2)
selectVariant(226-259)evaluateFlag(261-311)
apps/dashboard/lib/utils.ts (1)
apps/dashboard/lib/formatters.ts (1)
formatDate(64-83)
apps/dashboard/app/(main)/websites/[id]/flags/_components/variant-editor.tsx (1)
packages/shared/src/flags/index.ts (1)
Variant(34-34)
packages/shared/src/flags/utils.ts (2)
packages/redis/drizzle-cache.ts (1)
createDrizzleCache(31-249)packages/db/src/drizzle/schema.ts (1)
flags(640-708)
packages/sdk/src/node/flags/create-manager.ts (3)
packages/sdk/src/node/flags/index.ts (3)
createServerFlagsManager(2-2)ServerFlagsManager(1-1)createServerFlagsManagerInMemory(2-2)packages/sdk/src/core/flags/types.ts (1)
FlagsConfig(10-29)packages/sdk/src/node/flags/flags-manager.ts (1)
ServerFlagsManager(10-264)
apps/dashboard/app/(main)/websites/[id]/flags/_components/flags-list.tsx (2)
apps/dashboard/lib/flags/get-examples-strategy.ts (1)
getShouldShowExamples(68-78)apps/dashboard/app/(main)/websites/[id]/flags/_components/flag-examples.tsx (1)
FlagExamples(222-413)
apps/api/src/routes/public/flags.ts (1)
packages/db/src/drizzle/schema.ts (1)
flags(640-708)
packages/rpc/src/routers/flag-schedules.ts (3)
packages/rpc/src/orpc.ts (1)
protectedProcedure(34-61)packages/db/src/drizzle/schema.ts (2)
flags(640-708)flagSchedules(710-734)packages/shared/src/flags/index.ts (1)
flagScheduleSchema(97-182)
apps/dashboard/app/(main)/websites/[id]/flags/_components/types.ts (1)
packages/shared/src/flags/index.ts (2)
FlagType(36-36)Variant(34-34)
packages/rpc/src/routers/flags.ts (3)
packages/shared/src/flags/index.ts (2)
flagFormSchema(37-76)variantSchema(24-33)packages/db/src/drizzle/schema.ts (1)
flags(640-708)packages/shared/src/flags/utils.ts (2)
getScopeCondition(21-36)handleFlagUpdateDependencyCascading(50-156)
apps/dashboard/app/(main)/websites/[id]/flags/_components/flag-row.tsx (1)
apps/dashboard/app/(main)/websites/[id]/flags/_components/types.ts (1)
Flag(3-25)
apps/dashboard/components/providers/organizations-provider.tsx (2)
packages/auth/src/client/auth-client.ts (1)
authClient(24-42)apps/dashboard/stores/jotai/organizationsAtoms.ts (4)
organizationsAtom(4-4)activeOrganizationAtom(5-5)isLoadingOrganizationsAtom(6-6)getOrganizationBySlugAtom(8-11)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Vercel Agent Review
🔇 Additional comments (27)
packages/sdk/package.json (1)
14-17:⚠️ Cross-platform compatibility concern: pino (Node.js library) as a direct dependency in a multi-platform SDK.Adding
pinoas a direct runtime dependency in an SDK that exports to both browser environments (react, vue) and server environments (node, ai/vercel) may cause build failures, as pino is a Node.js-specific library with native bindings that cannot be bundled for browsers.Verify that:
- Pino is imported only in Node.js-specific code paths (e.g., under
src/node/,src/ai/vercel/).- Browser builds (react, vue, core) are properly tree-shaken or configured to exclude pino.
- Build configuration handles conditional imports per environment.
If pino is needed only in Node.js contexts, consider using conditional imports or moving it to an optional peer dependency instead of a direct dependency.
apps/dashboard/components/providers/organizations-provider.tsx (2)
14-16: LGTM!The formatting adjustment to the Organization type alias is purely cosmetic with no semantic changes.
70-73: Good addition of deprecation notice.The deprecation notice is a best practice for API migration. Verify that the new hook exists at
@/hooks/use-organizationsand that it exportsuseOrganizationsas documented.apps/dashboard/components/ui/switch.tsx (1)
4-4: Align Switch props typing with ref behavior usingforwardRef.Typing the component as
React.ComponentProps<typeof SwitchPrimitive.Root>on a plain function exposes arefprop in the type system, but function components withoutforwardRefcan't receive refs at runtime (leading to "Function components cannot be given refs" if consumers try). Wrap the component inReact.forwardRefand useComponentPropsWithoutRefto keep types and behavior consistent.Suggested refactor:
-import type * as React from "react"; +import * as React from "react"; @@ -function Switch({ - className, - ...props -}: React.ComponentProps<typeof SwitchPrimitive.Root>) { - return ( - <SwitchPrimitive.Root - className={cn( - "peer inline-flex h-[1.15rem] w-8 shrink-0 items-center rounded-full border border-transparent shadow-xs outline-none transition-all focus-visible:border-ring focus-visible:ring-[3px] focus-visible:ring-ring/50 disabled:cursor-not-allowed disabled:opacity-50 data-[state=checked]:bg-primary data-[state=unchecked]:bg-input dark:data-[state=unchecked]:bg-input/80 cursor-pointer", - className - )} - data-slot="switch" - {...props} - > - <SwitchPrimitive.Thumb - className={cn( - "pointer-events-none block size-4 rounded-full bg-background ring-0 transition-transform data-[state=checked]:translate-x-[calc(100%-2px)] data-[state=unchecked]:translate-x-0 dark:data-[state=checked]:bg-primary-foreground dark:data-[state=unchecked]:bg-foreground" - )} - data-slot="switch-thumb" - /> - </SwitchPrimitive.Root> - ); -} +const Switch = React.forwardRef< + React.ElementRef<typeof SwitchPrimitive.Root>, + React.ComponentPropsWithoutRef<typeof SwitchPrimitive.Root> +>(({ className, ...props }, ref) => ( + <SwitchPrimitive.Root + ref={ref} + className={cn( + "peer inline-flex h-[1.15rem] w-8 shrink-0 items-center rounded-full border border-transparent shadow-xs outline-none transition-all focus-visible:border-ring focus-visible:ring-[3px] focus-visible:ring-ring/50 disabled:cursor-not-allowed disabled:opacity-50 data-[state=checked]:bg-primary data-[state=unchecked]:bg-input dark:data-[state=unchecked]:bg-input/80 cursor-pointer", + className + )} + data-slot="switch" + {...props} + > + <SwitchPrimitive.Thumb + className={cn( + "pointer-events-none block size-4 rounded-full bg-background ring-0 transition-transform data-[state=checked]:translate-x-[calc(100%-2px)] data-[state=unchecked]:translate-x-0 dark:data-[state=checked]:bg-primary-foreground dark:data-[state=unchecked]:bg-foreground" + )} + data-slot="switch-thumb" + /> + </SwitchPrimitive.Root> +)); + +Switch.displayName = SwitchPrimitive.Root.displayName;Verify this matches your React version and Radix typings (e.g., that
ComponentPropsWithoutRefandElementRefresolve as expected in your TS config).turbo.json (1)
3-3: LGTM!Switching from
"tui"to"stream"mode is a valid configuration change. Stream mode outputs plain log streams instead of the interactive terminal UI, which can be preferable for CI/CD environments or when debugging build issues.packages/sdk/src/node/flags/index.ts (1)
1-2: LGTM!Clean barrel export consolidating the public API for server flag management. Good use of
typekeyword for the type-only export.packages/sdk/src/node/flags/flags-manager.ts (1)
1-32: LGTM on class structure and initialization.The constructor properly initializes defaults, sets up logging, and triggers async initialization without blocking. The
initPromisepattern allows consumers to await initialization when needed.packages/sdk/build.config.ts (1)
13-13: LGTM!The new build entry correctly includes the server-side flags module, consistent with the existing entry patterns.
packages/sdk/src/node/index.ts (1)
538-538: LGTM!The re-export correctly exposes the flags module through the main SDK node entry point.
apps/dashboard/app/(main)/websites/[id]/flags/_components/flag-row.tsx (2)
12-37: Formatting changes look good.The indentation and spacing adjustments improve consistency.
67-74: LGTM on computed values.The computed values for
ruleCount,rollout,isBooleanFlag, anddefaultLabelare correctly derived with appropriate type guards.apps/api/src/index.ts (1)
28-32: Missing scheduler cleanup on shutdown.The flag scheduler starts an interval but the signal handlers may not stop it. This could delay graceful shutdown or cause errors during cleanup. Verify that
stopFlagScheduleris called in theSIGINT/SIGTERMhandlers, or if no cleanup function exists, add one to properly clear the scheduler interval.packages/rpc/src/root.ts (1)
8-8: LGTM!The
flagSchedulesRouterimport and route registration follow the existing patterns in this file. The alphabetical ordering of imports and consistent naming convention are maintained.Also applies to: 27-27
packages/sdk/src/core/flags/types.ts (1)
28-28: LGTM!The optional
environmentproperty addition is backwards compatible and aligns with the environment-aware flag evaluation introduced across the PR.apps/api/src/routes/public/flags.test.ts (2)
457-547: LGTM! Comprehensive variant selection tests.The test suite thoroughly covers:
- Deterministic variant selection via user hash
- Sticky assignment consistency for the same user
- Support for different value types (string, number, object)
- Fallback to default when no variants exist
These tests effectively validate the core multivariant functionality.
549-580: LGTM! Multivariant evaluation tests are well-designed.The tests properly validate:
- Multivariant flags always return
enabled: true- Correct reason string
"MULTIVARIANT_EVALUATED"- Payload propagation
- Fallback behavior for non-multivariant flags
packages/shared/package.json (2)
31-32: LGTM!The new export paths for
./flagsand./flags/utilsfollow the existing naming conventions and provide clean public API surface for the flags functionality.
39-39: Verify that@databuddy/redisdependency is actually used in this package.The
@databuddy/redisdependency is added but appears unused in the@databuddy/sharedpackage source files. Unused dependencies increase bundle size and maintenance overhead. Search for any imports of@databuddy/redisinpackages/shared/src/and remove this dependency if not actively used.packages/sdk/src/node/flags/create-manager.ts (1)
1-42: LGTM! Clean factory pattern with good documentation.The factory functions are well-implemented with:
- Clear JSDoc documentation with practical code examples
- Backwards-compatible alias (
createServerFlagsManagerInMemory)- Simple, focused implementation
apps/dashboard/app/(main)/websites/[id]/flags/_components/types.ts (1)
1-2: LGTM! Good alignment with shared types.Importing
FlagTypeandVariantfrom@databuddy/shared/flagsensures type consistency across the codebase. The new optional properties (variants,dependencies,environment) properly extend theFlaginterface for multivariant support while maintaining backwards compatibility.Also applies to: 8-8, 14-16
packages/db/src/drizzle/schema.ts (1)
614-630: New enums look consistent with usage, no functional issues spotted
dbPermissionLevel, extendedflagType, andflagScheduleActionTypeare all aligned with how flags and schedules are used elsewhere (scheduler and RPC router). Nothing blocking here.packages/shared/src/flags/index.ts (2)
1-22: LGTM!The
userRuleSchemaproperly defines targeting rules with appropriate operators and optional fields for flexible rule configuration.
37-76: LGTM!The
flagFormSchemaprovides comprehensive validation with proper key format constraints and thesuperRefinecorrectly enforces that variant weights must sum to 100% when specified.apps/dashboard/app/(main)/websites/[id]/flags/_components/flag-sheet.tsx (2)
63-68: LGTM!The composite
flagWithScheduleSchemaproperly combines the flag form schema with an optional schedule, enabling unified form validation.
215-291: LGTM on submission flow structure.The submission logic properly handles create/update mutations, schedule creation, query invalidation, and error handling with appropriate user feedback.
packages/rpc/src/routers/flags.ts (2)
111-122: LGTM on schema composition.The
createFlagSchemaproperly spreadsflagFormSchema.shapeto inherit shared validation while adding scope-specific fields.
463-465: Consider error handling for cascading updates.If
handleFlagUpdateDependencyCascadingfails, the main flag update has already succeeded, potentially leaving dependent flags in an inconsistent state. Wrap this call in a try/catch with appropriate logging, or ensure it's part of a database transaction to maintain consistency.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
apps/dashboard/components/ui/switch.tsx (1)
3-4: Fix Radix Switch import path and namespace usage.
SwitchPrimitive.Root/SwitchPrimitive.Thumbwon’t exist withimport { Switch as SwitchPrimitive } from "radix-ui";and the bare"radix-ui"path is not a valid Radix React entry. Import from the scoped package and use a namespace import so.Rootand.Thumbresolve correctly.Suggested fix:
-import { Switch as SwitchPrimitive } from "radix-ui"; +import * as SwitchPrimitive from "@radix-ui/react-switch";Please also verify that
@radix-ui/react-switchis present in your dependencies and matches the Radix version used elsewhere in the project.Also applies to: 6-6
apps/dashboard/components/providers/organizations-provider.tsx (1)
77-88: Inline slug lookup is fine; remove unused getOrganizationBySlug atom usageThe new
getOrganizationimplementation usingorganizations.find((org) => org.slug === orgSlug)matches the previous selector behavior and is straightforward. However,getOrganizationBySlugAtomand its hook result are now unused (const [getOrganizationBySlug] = useAtom(getOrganizationBySlugAtom);), so they should be removed to avoid dead code and lints.You can apply the following diffs:
import { activeOrganizationAtom, - getOrganizationBySlugAtom, isLoadingOrganizationsAtom, organizationsAtom, } from "@/stores/jotai/organizationsAtoms";export function useOrganizationsContext() { const organizations = useAtomValue(organizationsAtom); const activeOrganization = useAtomValue(activeOrganizationAtom); const isLoading = useAtomValue(isLoadingOrganizationsAtom); - const [getOrganizationBySlug] = useAtom(getOrganizationBySlugAtom); return { organizations, activeOrganization, isLoading, getOrganization: (orgSlug: string) => organizations.find((org) => org.slug === orgSlug), }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
apps/dashboard/components/providers/organizations-provider.tsx(2 hunks)apps/dashboard/components/ui/switch.tsx(1 hunks)packages/db/src/drizzle/schema.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
apps/dashboard/components/ui/switch.tsx (1)
apps/dashboard/lib/utils.ts (1)
cn(5-7)
apps/dashboard/components/providers/organizations-provider.tsx (2)
packages/auth/src/client/auth-client.ts (1)
authClient(24-42)apps/dashboard/stores/jotai/organizationsAtoms.ts (4)
organizationsAtom(4-4)activeOrganizationAtom(5-5)isLoadingOrganizationsAtom(6-6)getOrganizationBySlugAtom(8-11)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Vercel Agent Review
🔇 Additional comments (9)
apps/dashboard/components/ui/switch.tsx (3)
1-1: Client directive on a Radix-powered component is appropriate.Marking this as a client component is necessary for an interactive Switch in a Next.js app; no issues here.
9-11: Props typing andclassNamehandling look correct.Destructuring
classNameand spreading the rest asReact.ComponentProps<typeof SwitchPrimitive.Root>keeps the wrapper aligned with the underlying Radix props while allowing class merging viacn.
17-20: Switch root className changes are consistent and non-breaking.Adding
cursor-pointerwhile keepingdisabled:cursor-not-allowedand the existing data-state variants preserves disabled styling and improves hover/interaction affordance for the enabled state.packages/db/src/drizzle/schema.ts (4)
621-621: LGTM!The addition of
"multivariant"to theflagTypeenum aligns with the newvariantscolumn in theflagstable and supports the multivariant flag feature.
712-736: Table structure looks sound for scheduling use cases.The
flagSchedulestable correctly supports both single-shot schedules (scheduledAt) and multi-step rollouts (rolloutSteps). The cascade delete onflagIdensures cleanup when flags are removed.As noted in prior feedback, a composite index on
(isEnabled, executedAt, scheduledAt)may help scheduler query performance if load increases, but the current indexes are adequate for initial use.
616-619: VerifydbPermissionLevelenum is used in the codebase.The enum is defined but not referenced in this schema file. Confirm it's actually used elsewhere (e.g., in table definitions, API logic, or other modules) before merging.
660-662: Consider referential integrity fordependenciescolumn.The
dependenciescolumn stores flag IDs as a text array without foreign key constraints. If a referenced flag is deleted, orphaned references will remain. Verify that:
- Flag evaluation logic handles missing dependencies gracefully
- Application-level validation prevents orphaned references when flags are deleted
- The design choice to avoid FK constraints is intentional and documented
apps/dashboard/components/providers/organizations-provider.tsx (2)
14-16: Organization type alias looks correct and preciseDeriving
OrganizationasNonNullable<ReturnType<typeof authClient.useListOrganizations>["data"]>[number]cleanly captures the array element type and should stay in sync with the auth client’s hook return type; no issues from a typing perspective.
72-75: Deprecation JSDoc is well-placedThe
@deprecatedJSDoc is correctly attached touseOrganizationsContext, so TypeScript tooling and editors will surface the guidance to migrate touseOrganizationswhile preserving backward compatibility.
There was a problem hiding this comment.
Additional Suggestion:
Attempting to call waitForInitialization() on the factory function instead of the created instance will cause a runtime error.
View Details
📝 Patch Details
diff --git a/apps/dashboard/lib/flags/get-examples-strategy.ts b/apps/dashboard/lib/flags/get-examples-strategy.ts
index 3a863962..0f45711d 100644
--- a/apps/dashboard/lib/flags/get-examples-strategy.ts
+++ b/apps/dashboard/lib/flags/get-examples-strategy.ts
@@ -73,7 +73,7 @@ export const getShouldShowExamples = async (websiteId: string, userId: string, e
debug: process.env.NODE_ENV === "development",
environment,
});
- await createServerFlagsManager.waitForInitialization()
+ await flagsManager.waitForInitialization()
const flag = await flagsManager.getFlag("enable-flag-examples");
return flag.value;
}
\ No newline at end of file
Analysis
Incorrect method call on factory function instead of instance
What fails: getShouldShowExamples() in apps/dashboard/lib/flags/get-examples-strategy.ts calls await createServerFlagsManager.waitForInitialization() on the factory function instead of the created manager instance.
How to reproduce: Call getShouldShowExamples() function with any websiteId and userId parameters.
What happens: Runtime error: createServerFlagsManager.waitForInitialization is not a function
Expected behavior: Should call await flagsManager.waitForInitialization() on the instance returned by createServerFlagsManager(), not on the factory function itself. The waitForInitialization() method is defined on the ServerFlagsManager class (packages/sdk/src/node/flags/flags-manager.ts, line 46), not on the factory function (packages/sdk/src/node/flags/create-manager.ts).
Root cause: The first function getExamplesDisplayStrategy() correctly calls await flagsManager.waitForInitialization() after creating the instance, but getShouldShowExamples() incorrectly calls the method on the factory function directly.
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (12)
packages/rpc/src/routers/flag-schedules.ts (2)
10-36: Missing authorization checks for organization and user-scoped flags.The
getByFlagIdhandler only authorizes access whenflag.websiteIdexists. Flags scoped toorganizationIdoruserIdbypass authorization entirely, allowing any authenticated user to view schedules for flags they don't own.This same issue affects
create,update, anddeletehandlers.Apply a unified authorization helper:
+async function authorizeFlagAccess( + context: Context, + flag: { websiteId: string | null; organizationId: string | null; userId: string | null }, + permission: "read" | "update" = "read" +) { + if (flag.websiteId) { + await authorizeWebsiteAccess(context, flag.websiteId, permission); + } else if (flag.organizationId) { + // Add organization permission check + } else if (flag.userId && flag.userId !== context.user?.id && context.user?.role !== "ADMIN") { + throw new ORPCError("FORBIDDEN", { message: "Not authorized to access this flag" }); + } +}
31-35: Consider returningnullinstead of throwing when no schedules exist.The endpoint throws
NOT_FOUNDwhen a flag exists but has no schedules. This may cause unnecessary error handling in the frontend for a valid state (flag exists, no schedules yet).- if (!schedules[0]) { - throw new ORPCError("NOT_FOUND", { message: "No schedules found for this flag" }); - } - - return schedules[0]; + return schedules[0] ?? null;packages/sdk/src/node/flags/flags-manager.ts (2)
114-120: Incomplete user comparison may return stale cached results.The
isDifferentUsercheck only comparesuserId, ignoringproperties. If targeting rules depend on email or properties, the cached value could be incorrect for users with the sameuserIdbut different attributes.- const isDifferentUser = user && (user.userId !== this.config.user?.userId); + const isDifferentUser = user && ( + user.userId !== this.config.user?.userId || + user.email !== this.config.user?.email || + JSON.stringify(user.properties) !== JSON.stringify(this.config.user?.properties) + );
171-177:isDefaultUsercheck has the same incomplete comparison issue.This check also only compares
userId, which could lead to incorrect caching behavior. Consider extracting a reusableisSameUserhelper for both checks.apps/dashboard/lib/flags/get-examples-strategy.ts (2)
41-41: Remove debugconsole.logstatement.This unconditional
console.logwill output flag results in production. Either remove it or gate behind a debug check.- console.log("🚀 Flag result:", result); + // Removed debug logging - use logger.debug if needed
68-68: Add explicit return type annotation for consistency.Unlike
getExamplesDisplayStrategy, this function lacks an explicit return type annotation.-export const getShouldShowExamples = async (websiteId: string, userId: string, environment: string) => { +export const getShouldShowExamples = async (websiteId: string, userId: string, environment: string): Promise<boolean> => {packages/shared/src/flags/utils.ts (2)
21-36: Clarify or improve fallback behavior ingetScopeCondition.The fallback
eq(flags.organizationId, "")returns a "match nothing" condition since organizationId is unlikely to be an empty string. While this may be intentional for safety, consider either documenting this behavior or throwing an error if no valid scope is provided.if (userId) { return eq(flags.userId, userId); } - return eq(flags.organizationId, ""); + // Safety fallback: match nothing if no scope provided + // Consider throwing an error here if this indicates a bug + return eq(flags.organizationId, "");
129-139: Consider batching DB updates by status.Each flag update triggers a separate
db.update()call. For better performance with multiple flags, consider grouping bynewStatusand usinginArray:- await Promise.all( - flagsToUpdate.map((flagUpdate) => - db - .update(flags) - .set({ - status: flagUpdate.newStatus, - updatedAt: new Date(), - }) - .where(eq(flags.id, flagUpdate.id)) - ) - ); + const toInactive = flagsToUpdate.filter(f => f.newStatus === 'inactive').map(f => f.id); + const toActive = flagsToUpdate.filter(f => f.newStatus === 'active').map(f => f.id); + const now = new Date(); + + if (toInactive.length > 0) { + await db.update(flags) + .set({ status: 'inactive', updatedAt: now }) + .where(inArray(flags.id, toInactive)); + } + if (toActive.length > 0) { + await db.update(flags) + .set({ status: 'active', updatedAt: now }) + .where(inArray(flags.id, toActive)); + }packages/shared/src/flags/index.ts (2)
85-93: Consider validatingscheduledAtas a datetime string at the schema level.The
scheduledAtfield accepts any string. Usingz.string().datetime()would catch invalid datetime formats earlier, before thesuperRefinevalidation runs.export const rolloutStepSchema = z.object({ - scheduledAt: z.string(), + scheduledAt: z.string().datetime({ message: "Invalid datetime format" }), executedAt: z.string().optional(),
155-179: Consider validating that rollout steps are in chronological order.The validation checks that each step's
scheduledAtis in the future, but doesn't verify that steps are ordered chronologically. Out-of-order steps could cause unexpected rollout behavior.for (const step of data.rolloutSteps) { + const stepDate = new Date(step.scheduledAt); + if (isNaN(stepDate.getTime())) { + ctx.addIssue({ + code: "custom", + path: ["rolloutSteps"], + message: "Invalid date in rollout step", + }); + continue; + } if (typeof step.value !== "number") { // ... existing validation } } + + // Validate chronological order + const dates = data.rolloutSteps.map(s => new Date(s.scheduledAt).getTime()); + for (let i = 1; i < dates.length; i++) { + if (dates[i] <= dates[i - 1]) { + ctx.addIssue({ + code: "custom", + path: ["rolloutSteps"], + message: "Rollout steps must be in chronological order", + }); + break; + } + }packages/rpc/src/routers/flags.ts (2)
320-377: Create‑time dependency checks andfinalStatuscalculation look good; minor nit onenvironmentfallbackThe new logic in
create:
- Runs
checkCircularDependencybefore mutating state.- Loads dependency flags within the same scope and forces
finalStatus = "inactive"when any dependency is non‑active.- Reuses
finalStatusconsistently in both restore and insert branches.That all looks sound and matches the intended “dependencies gate activation” semantics.
One minor readability nit already raised in a previous review: in the
insertbranchexistingFlagis always empty, so:environment: input.environment || existingFlag?.[0]?.environment,is effectively equivalent to
input.environment || null. It’s harmless but slightly misleading, since the fallback can never actually be hit in this path.If you want to tidy it up:
- environment: input.environment || existingFlag?.[0]?.environment, + environment: input.environment || null,The restore path where
existingFlag[0]is defined can keep using the real fallback.Also applies to: 413-435
471-508: Dependency status enforcement onupdatehas a logic gap and still mutatesinput.statusTwo issues in this block:
Existing dependencies are ignored when only
statusis updated.
dependencyFlagsis fetched usinginput.dependencies || [], butnextDependenciesfalls back toflag.dependencieswheninput.dependenciesis undefined:const dependencyFlags = await context.db .select() .from(flags) .where( and( inArray(flags.key, input.dependencies || []), getScopeCondition(...), isNull(flags.deletedAt) ) ); const nextDependencies = input.dependencies ?? (flag.dependencies as string[]) ?? []; if (nextDependencies.length > 0 && input.status === "active") { const hasInactiveDependency = dependencyFlags.some( (depFlag) => depFlag.status !== "active" ); // ... }If a flag already has dependencies and you call
updatewithstatus: "active"but nodependenciesfield,nextDependenciescontains the existing dependencies, butdependencyFlagsis computed from[], so inactive dependencies are never detected and the flag can be set toactivein violation of your gating rule.
input.statusis still mutated in place, which was already called out in a prior review and is generally undesirable for request inputs:if (hasInactiveDependency) { input.status = "inactive"; }A small restructuring fixes both:
- Always reason about a local
finalStatus.- Base the dependency query on
nextDependencies(the effective dependencies after the update).- Only hit the DB when there are dependencies and the effective status is (or is being changed to)
"active".For example:
- const dependencyFlags = await context.db - .select() - .from(flags) - .where( - and( - inArray(flags.key, input.dependencies || []), - getScopeCondition( - flag.websiteId || undefined, - flag.organizationId || undefined, - context.user.id - ), - isNull(flags.deletedAt) - ) - ); - - const nextDependencies = input.dependencies ?? (flag.dependencies as string[]) ?? []; - - if (nextDependencies.length > 0 && input.status === "active") { - const hasInactiveDependency = dependencyFlags.some( - (depFlag) => depFlag.status !== "active" - ); - - if (hasInactiveDependency) { - input.status = "inactive"; - } - } - - const { id, ...updates } = input; - const [updatedFlag] = await context.db - .update(flags) - .set({ - ...updates, - updatedAt: new Date(), - }) + const nextDependencies = + input.dependencies ?? (flag.dependencies as string[]) ?? []; + + let finalStatus = input.status; + + if (nextDependencies.length > 0) { + const effectiveStatus = (finalStatus ?? flag.status) as + | "active" + | "inactive" + | "archived"; + + if (effectiveStatus === "active") { + const dependencyFlags = await context.db + .select() + .from(flags) + .where( + and( + inArray(flags.key, nextDependencies), + getScopeCondition( + flag.websiteId || undefined, + flag.organizationId || undefined, + context.user.id + ), + isNull(flags.deletedAt) + ) + ); + + const hasInactiveDependency = dependencyFlags.some( + (depFlag) => depFlag.status !== "active" + ); + + if (hasInactiveDependency) { + finalStatus = "inactive"; + } + } + } + + const { id, ...updates } = input; + const [updatedFlag] = await context.db + .update(flags) + .set({ + ...updates, + status: finalStatus ?? updates.status, + updatedAt: new Date(), + })This keeps the invariant “a flag with any inactive dependencies cannot be active” regardless of whether the caller supplies
dependenciesin the update, and avoids mutating the validated input object.Also applies to: 509-515
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
apps/dashboard/app/(main)/websites/[id]/flags/_components/schedule-manager.tsx(1 hunks)apps/dashboard/lib/flags/get-examples-strategy.ts(1 hunks)apps/dashboard/lib/utils.ts(1 hunks)packages/rpc/src/routers/flag-schedules.ts(1 hunks)packages/rpc/src/routers/flags.ts(12 hunks)packages/sdk/src/node/flags/flags-manager.ts(1 hunks)packages/shared/src/flags/index.ts(1 hunks)packages/shared/src/flags/utils.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
apps/dashboard/lib/flags/get-examples-strategy.ts (2)
packages/sdk/src/node/flags/create-manager.ts (1)
createServerFlagsManager(19-23)packages/sdk/src/node/flags/index.ts (1)
createServerFlagsManager(2-2)
packages/shared/src/flags/utils.ts (2)
packages/redis/drizzle-cache.ts (1)
createDrizzleCache(31-249)packages/db/src/drizzle/schema.ts (1)
flags(642-710)
apps/dashboard/app/(main)/websites/[id]/flags/_components/schedule-manager.tsx (3)
apps/dashboard/components/ui/switch.tsx (1)
Switch(32-32)apps/dashboard/lib/utils.ts (1)
cn(5-7)apps/dashboard/lib/formatters.ts (1)
DATE_FORMATS(52-61)
packages/sdk/src/node/flags/flags-manager.ts (2)
packages/sdk/src/node/flags/index.ts (1)
ServerFlagsManager(1-1)packages/sdk/src/core/flags/types.ts (5)
FlagsManager(60-69)FlagsConfig(10-29)FlagResult(1-8)FlagsManagerOptions(53-58)FlagState(31-35)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Vercel Agent Review
🔇 Additional comments (13)
apps/dashboard/lib/utils.ts (1)
49-51: Verify that the file content is complete and that past issues have been resolved.The annotated code provided ends at line 51 (the
getDeviceIconfunction), but past review comments reference aformatDatefunction and localization concerns around lines 53–79. These flagged issues are not visible in the current code snippet.This could indicate that:
- The
formatDatefunction has been removed or refactored- The annotated code snippet is incomplete
- Those concerns have already been addressed in a recent commit
Please confirm the full file has been provided and whether the past flagged issues (code duplication with
formatters.tsand the.replace(',', ' at')localization concern) have been resolved or remain pending.apps/dashboard/app/(main)/websites/[id]/flags/_components/schedule-manager.tsx (2)
60-90: Confirm whether disabling should wipe the entire schedule object.When the switch is turned off, the code calls
setValue("schedule", undefined), permanently discarding all configured type/time/rollout data. Re-enabling requires rebuilding the entire configuration from scratch. If the intent is to pause/resume a schedule, consider toggling onlyschedule.isEnabledand leaving the rest ofscheduleintact.
134-201: Make the time input controlled and drop redundantupdate_rolloutchecks.The entire
FormFieldis already wrapped in{watchedScheduledType !== "update_rollout" && ...}, making thedisabled={watchedScheduledType === "update_rollout"}checks on the FormField and Button redundant—remove them for clarity.The
<Input type="time">usesdefaultValue, which only applies on mount. Iffield.valuechanges after the component mounts (e.g., via Calendar selection or initial data load), the time input won't reflect that change. Convert it to a controlled component usingvalueinstead.- <FormField - control={form.control} - name="schedule.scheduledAt" - disabled={watchedScheduledType === "update_rollout"} + <FormField + control={form.control} + name="schedule.scheduledAt" render={({ field }) => ( <FormItem className="flex flex-col grow"> <FormLabel>Date & Time</FormLabel> <Popover> <PopoverTrigger asChild> <FormControl> <Button variant="outline" className={cn( "w-full pl-3 text-left font-normal", !field.value && "text-muted-foreground" )} - disabled={watchedScheduledType === "update_rollout"} > {field.value ? formatDate(new Date(field.value), DATE_FORMATS.DATE_TIME_12H) : "Pick a Time"} <CalendarIcon className="ml-auto h-4 w-4 opacity-50" /> </Button> </FormControl> </PopoverTrigger> <PopoverContent className="w-auto p-0" align="start"> <Calendar mode="single" selected={ field.value ? new Date(field.value) : undefined } onSelect={(date) => { if (date) { const currentTime = field.value ? new Date(field.value) : new Date(); date.setHours(currentTime.getHours()); date.setMinutes(currentTime.getMinutes()); field.onChange(date.toISOString()); } }} /> <div className="p-3 border-t"> <Input type="time" - defaultValue={ - field.value - ? formatDate(new Date(field.value), DATE_FORMATS.TIME_ONLY) - : "" - } + value={ + field.value + ? formatDate(new Date(field.value), DATE_FORMATS.TIME_ONLY) + : "" + } onChange={(e) => { const date = field.value ? new Date(field.value) : new Date(); const [h, m] = e.target.value.split(":"); date.setHours(Number(h), Number(m)); field.onChange(date.toISOString()); }} /> </div> </PopoverContent> </Popover> <FormMessage /> </FormItem> )} />}packages/rpc/src/routers/flag-schedules.ts (1)
119-142: Delete handler logic is correct.The handler now properly looks up the schedule first, then retrieves the associated flag using
existingSchedule.flagIdfor authorization. This addresses the previous critical issue whereinput.idwas incorrectly used for flag lookup.packages/sdk/src/node/flags/flags-manager.ts (2)
58-99: Bulk fetch now includes theenvironmentparameter.The
fetchAllFlagsmethod correctly includes theenvironmentparameter in the query string (lines 77-79), ensuring consistency with the single flag fetch behavior.
237-250: Unhandled promise rejections are now properly caught.Both
updateUserandupdateConfigmethods now handle promise rejections fromrefresh()andfetchAllFlags()with.catch()handlers.packages/shared/src/flags/utils.ts (3)
9-19: Cache invalidation now usesPromise.allSettled.This correctly handles partial failures during cache invalidation, ensuring one failure doesn't prevent other invalidations from completing.
95-124: Dependency resolution now uses batched queries.The implementation correctly batches dependency lookups using
inArrayinstead of individual queries per flag, addressing the N+1 query concern. The check now also validates that dependencies exist (viadepFlagsByKey.get(key)returning undefined for missing keys).
154-162: Recursive cascading for transitive dependencies is implemented.The cascading logic now recursively calls
handleFlagUpdateDependencyCascadingfor each updated flag, ensuring transitive dependencies (A→B→C) are properly handled.packages/shared/src/flags/index.ts (2)
24-34: Variant schema type consistency is improved.The
typeenum now correctly includes only"string"and"number", which aligns with thevaluefield acceptingz.string()andz.number(). This resolves the previous type mismatch where"json"was declared but objects weren't accepted.
62-76: Multivariant weight validation is well-implemented.The validation correctly checks that weights sum to 100% only when at least one variant has a weight specified, allowing for implicit equal distribution when weights are omitted.
packages/rpc/src/routers/flags.ts (2)
140-200: New DFS‑basedcheckCircularDependencycorrectly handles transitive cyclesThe helper now builds a dependency graph across all flags in the scope and uses a standard DFS + recursion stack to detect cycles reachable from
targetFlagKey. This closes the previous gap where only direct cycles were detected (e.g. A→B→A) and now also catches longer chains (A→B→C→A).Implementation looks correct and side‑effect free for non‑cyclic graphs; complexity is linear in the number of flags in scope, which is reasonable here.
124-138: Remove.passthrough()fromupdateFlagSchema– this enables mass-assignment into DB updatesWith
.passthrough(), any extra keys sent by a client will be forwarded into the database update, including potentially sensitive internal columns. For an external API surface, the schema should be strict.Remove
.passthrough()and explicitly add any legitimately updatable fields (e.g.,environmentif needed) directly to the schema.const updateFlagSchema = z.object({ id: z.string(), name: z.string().min(1).max(100).optional(), description: z.string().optional(), type: z.enum(["boolean", "rollout", "multivariant"]).optional(), status: z.enum(["active", "inactive", "archived"]).optional(), defaultValue: z.boolean().optional(), payload: z.any().optional(), rules: z.array(userRuleSchema).optional(), persistAcrossAuth: z.boolean().optional(), rolloutPercentage: z.number().min(0).max(100).optional(), variants: z.array(variantSchema).optional(), dependencies: z.array(z.string()).optional(), forceCancelScheduledRollout: z.boolean().optional(), -}).passthrough(); +});
There was a problem hiding this comment.
Actionable comments posted: 11
♻️ Duplicate comments (14)
apps/api/src/routes/public/flags.ts (1)
76-97: Bulk evaluation doesn't filter by environment.
getCachedFlagsForClientdoesn't accept an environment parameter, so/bulkendpoint returns all flags regardless of environment. If environment scoping should apply consistently, consider extending bulk evaluation to accept and filter by environment.apps/dashboard/lib/flags/get-examples-strategy.ts (2)
41-41: Remove or gate debug logging in production.The
console.logstatement at line 41 runs unconditionally in production. Either remove it or wrap it in a debug check.- console.log("🚀 Flag result:", result); + if (process.env.NODE_ENV === "development") { + console.log("🚀 Flag result:", result); + }
27-36: Consider caching the flags manager instance.Creating a new
ServerFlagsManagerand awaiting initialization on every request is inefficient, especially in serverless environments. Consider module-level caching keyed bywebsiteIdandenvironmentto avoid repeated cold-start initialization overhead.Also applies to: 68-78
apps/dashboard/app/(main)/websites/[id]/flags/_components/flags-list.tsx (2)
47-50: Critical: Hardcoded websiteId, userId, and environment values.The
getShouldShowExamplescall uses hardcoded test values instead of deriving them from route params and session context. This causes the feature to query the wrong website's flag data for all users.+"use client"; + +import { useParams } from "next/navigation"; +import { useSession } from "@databuddy/auth/client"; +// ... other imports + export function FlagsList({ flags, isLoading, onCreateFlagAction, onEditFlagAction, }: FlagsListProps) { + const { id: websiteId } = useParams<{ id: string }>(); + const { data: session } = useSession(); const [searchQuery, setSearchQuery] = useState(""); const [statusFilter, setStatusFilter] = useState<FlagStatus | "all">("all"); const [showExamples, setShowExamples] = useState(false); const isFlagExamplesEnabledQuery = useQuery({ - queryKey: ["isFlagExamplesEnabled"], - queryFn: () => getShouldShowExamples("OSA-FWQhcahi6J5VDxsbn", "", "production"), + queryKey: ["isFlagExamplesEnabled", websiteId, session?.user?.id], + queryFn: () => getShouldShowExamples( + websiteId, + session?.user?.id || "", + process.env.NODE_ENV || "development" + ), + enabled: Boolean(websiteId), });
140-148: TODO comment indicates incomplete implementation.The template data from
onCreateFromTemplateis not being used to pre-fill the create form.Would you like me to help implement the auto-fill functionality for the create form using the template data?
packages/db/src/drizzle/schema.ts (1)
628-632: Naming inconsistency between TypeScript variable and database enum.The TypeScript export is
flagScheduleActionType, but the database enum is named"flag_schedule_type". Consider aligning them for consistency.apps/dashboard/app/(main)/websites/[id]/flags/_components/variant-editor.tsx (1)
264-294: Weight status incorrectly shows "Even Distribution" when weights exist but sum to 0.If a user enables weights on variants but sets them all to
0,weightedVariants.length > 0yettotalWeight === 0, so the UI shows "Even Distribution" even though weighted selection is active. The condition should check whether any explicit weights exist, not whether the total is zero.+ const hasExplicitWeights = weightedVariants.length > 0; + return ( // ... <div className={`text-sm flex items-center gap-2 ${ - totalWeight === 0 + !hasExplicitWeights ? "text-blue-600" : isValidTotal ? "text-green-600" : "text-amber-600" }`} > <div className={`w-2 h-2 rounded-full ${ - totalWeight === 0 + !hasExplicitWeights ? "bg-blue-600" : isValidTotal ? "bg-green-600" : "bg-amber-600" }`} /> - {totalWeight === 0 ? ( + {!hasExplicitWeights ? ( <> <span className="font-medium">Even Distribution</span>apps/dashboard/app/(main)/websites/[id]/flags/_components/schedule-manager.tsx (1)
131-133: Unreachable code:update_rolloutcheck can never trigger.The
<SelectItem value="update_rollout">is commented out (lines 124-128), sowatchedScheduledTypecan never be"update_rollout". This FormDescription block is dead code.- {watchedScheduledType === "update_rollout" && <FormDescription> - Schedule different rollouts for your traffic. - </FormDescription>}apps/api/src/services/flag-scheduler.ts (1)
111-111: Avoidanytype for updates object.Using
anyloses type safety during construction. Consider using Drizzle's inferred types for better compile-time checks.+import type { InferInsertModel } from 'drizzle-orm'; + - const updates: any = { updatedAt: new Date() }; + const updates: Partial<InferInsertModel<typeof flags>> = { updatedAt: new Date() };packages/shared/src/flags/index.ts (2)
85-93: Consider validatingscheduledAtas a datetime string at the schema level.The
scheduledAtfield accepts any string. Usingz.string().datetime()would ensure valid ISO date format at the schema level, providing clearer validation errors earlier in the parsing pipeline.export const rolloutStepSchema = z.object({ - scheduledAt: z.string(), + scheduledAt: z.string().datetime({ message: "Invalid datetime format" }), executedAt: z.string().optional(), value: z.union([ z.number().min(0).max(100), z.literal("enable"), z.literal("disable"), ]), });Based on past review comments.
147-180: Incomplete validation forrolloutSteps.The validation checks that step values are numbers and that
scheduledAtis in the future, but doesn't validate:
- Each step's
scheduledAtis a valid parseable date (before comparing toDate.now())- Steps are scheduled in chronological order
This could lead to runtime errors (invalid date parsing) or logically invalid schedules being created.
for (const step of data.rolloutSteps) { + const stepDate = new Date(step.scheduledAt); + if (isNaN(stepDate.getTime())) { + ctx.addIssue({ + code: "custom", + path: ["rolloutSteps"], + message: "Invalid date in rollout step", + }); + continue; + } + if (stepDate.getTime() <= Date.now()) { + ctx.addIssue({ + code: "custom", + path: ["rolloutSteps"], + message: "Rollout step must be scheduled in the future", + }); + } if (typeof step.value !== "number") { ctx.addIssue({ code: "custom", path: ["value"], message: "Step value must be a number between 0 and 100 for rollout steps", }); - + continue; } + } + // Validate chronological order + for (let i = 1; i < data.rolloutSteps.length; i++) { + const prevDate = new Date(data.rolloutSteps[i - 1].scheduledAt).getTime(); + const currDate = new Date(data.rolloutSteps[i].scheduledAt).getTime(); + if (currDate <= prevDate) { + ctx.addIssue({ + code: "custom", + path: ["rolloutSteps", i, "scheduledAt"], + message: "Rollout steps must be scheduled in chronological order", + }); + } }Based on past review comments.
apps/dashboard/app/(main)/websites/[id]/flags/_components/flag-sheet.tsx (3)
180-198: Missingformin useEffect dependency array.The effect calls
form.setValuebut doesn't includeformin the dependency array. While theformobject fromuseFormis stable, adding it would satisfy the exhaustive-deps linter rule and ensure correctness.- }, [watchedName, keyManuallyEdited, isEditing, watchedIsScheduleEnabled]); + }, [watchedName, keyManuallyEdited, isEditing, watchedIsScheduleEnabled, form, flag?.id]);Based on past review comments.
218-257: Avoidanytype for mutation data.Using
anytype (lines 218, 239, 244) bypasses TypeScript's type checking and could lead to runtime errors if mutations expect different shapes. Consider defining proper types or inferring from the mutation's expected input.Based on past review comments.
753-760: Avoid direct mutation of nested form state.The code creates a shallow copy of
watchedRolloutStepsbut then mutates the step object at indexidxdirectly (newSteps[idx].scheduledAt = ...). This can cause React state inconsistencies and missed re-renders. Create a new object for the modified step.onSelect={(date) => { if (date) { ensureScheduleFields(); - const newSteps = [...(watchedRolloutSteps || [])]; - newSteps[idx].scheduledAt = date.toISOString(); + const newSteps = (watchedRolloutSteps || []).map((s, i) => + i === idx ? { ...s, scheduledAt: date.toISOString() } : s + ); form.setValue("schedule.rolloutSteps", newSteps); } }}Apply the same pattern at lines 780-782 and 799-801.
Based on past review comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
apps/api/src/routes/public/flags.ts(6 hunks)apps/api/src/services/flag-scheduler.ts(1 hunks)apps/dashboard/app/(main)/websites/[id]/flags/_components/dependency-selector.tsx(1 hunks)apps/dashboard/app/(main)/websites/[id]/flags/_components/flag-sheet.tsx(1 hunks)apps/dashboard/app/(main)/websites/[id]/flags/_components/flags-list.tsx(1 hunks)apps/dashboard/app/(main)/websites/[id]/flags/_components/schedule-manager.tsx(1 hunks)apps/dashboard/app/(main)/websites/[id]/flags/_components/variant-editor.tsx(1 hunks)apps/dashboard/lib/flags/get-examples-strategy.ts(1 hunks)packages/db/src/drizzle/schema.ts(4 hunks)packages/shared/src/flags/index.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
apps/api/src/services/flag-scheduler.ts (3)
packages/redis/drizzle-cache.ts (1)
createDrizzleCache(31-249)packages/db/src/drizzle/schema.ts (2)
flagSchedules(714-738)flags(644-712)packages/shared/src/flags/utils.ts (1)
handleFlagUpdateDependencyCascading(50-166)
apps/dashboard/lib/flags/get-examples-strategy.ts (2)
packages/sdk/src/node/flags/create-manager.ts (1)
createServerFlagsManager(19-23)packages/sdk/src/node/flags/index.ts (1)
createServerFlagsManager(2-2)
apps/dashboard/app/(main)/websites/[id]/flags/_components/variant-editor.tsx (2)
packages/shared/src/flags/index.ts (1)
Variant(34-34)apps/api/src/query/expressions.ts (1)
field(414-418)
apps/dashboard/app/(main)/websites/[id]/flags/_components/schedule-manager.tsx (3)
packages/shared/src/flags/index.ts (1)
FlagWithScheduleForm(191-191)apps/dashboard/lib/utils.ts (1)
cn(5-7)apps/dashboard/lib/formatters.ts (1)
DATE_FORMATS(52-61)
apps/api/src/routes/public/flags.ts (2)
packages/redis/cacheable.ts (1)
cacheable(64-126)packages/db/src/drizzle/schema.ts (1)
flags(644-712)
packages/shared/src/flags/index.ts (1)
packages/db/src/drizzle/schema.ts (1)
RolloutStep(633-635)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Vercel Agent Review
🔇 Additional comments (8)
apps/api/src/services/flag-scheduler.ts (2)
137-152: LGTM: Rollout step marking logic correctly handles missed steps.The logic at lines 139-144 correctly marks all steps with
scheduledAt <= nowas executed (not just the latest one), which prevents the replay issue where older steps could revert newer changes after server downtime. This addresses the previously raised concern.
158-159: VerifyuserIdis available in schedule for dependency cascading.
handleFlagUpdateDependencyCascadingreceivessched.userId, butExecutableSchedule.userIdis optional and theflagSchedulestable may not have auserIdcolumn. This could passundefinedand affect dependency resolution. Confirm thatuserIdis populated at the call site and that the function handles optional values appropriately.apps/api/src/routes/public/flags.ts (2)
246-257: Weighted selection may select unintended variant when weights don't sum to 100.If weighted variants sum to less than 100 (e.g., two variants at 30% each = 60% total), users with
percentagevalues between 60-99 will fall through the loop and always get the last variant. This may be intentional as a catch-all, but could lead to unexpected distribution.Consider whether this fallback behavior is desired or if weights should be validated/normalized:
// Option 1: Normalize weights (if you want proportional distribution) const totalConfiguredWeight = flag.variants.reduce((sum, v) => sum + (v.weight || 0), 0); const normalizedPercentage = (percentage / 100) * totalConfiguredWeight; // Option 2: Document the fallback behavior clearly // Current behavior: Users not covered by explicit weights get the last variant
40-46: LGTM: Multivariant flag evaluation correctly implemented.The
FlagResulttype now supportsvalue: boolean | string | numberwith optionalvariantfield, and the multivariant evaluation path correctly returns the selected variant with appropriate reason.Also applies to: 274-283
apps/dashboard/app/(main)/websites/[id]/flags/_components/flag-sheet.tsx (3)
96-114: LGTM! Proper form setup with composite schema.The form correctly uses
FlagWithScheduleFormderived fromflagWithScheduleSchema, properly nesting flag and schedule fields. The default values are well-structured and type-safe.
682-704: LGTM! Rollout step defaults to future time.The new rollout step now defaults to 1 hour in the future (
Date.now() + 60 * 60 * 1000), which correctly satisfies the schema validation requirement that scheduled times must be in the future. This addresses the concern from previous reviews.
830-906: LGTM! Well-structured integration of new components.The integration of
VariantEditor,DependencySelector, andScheduleManageris clean and properly wired to the form. The conditional rendering based on flag type (multivariant, rollout, boolean) and the dependency validation logic (preventing activation when dependencies are inactive) are well-implemented.packages/shared/src/flags/index.ts (1)
85-95: Verify RolloutStep type consistency with database schema.The
RolloutSteptype exported here differs from the one inpackages/db/src/drizzle/schema.ts:
- Database schema: Discriminated union with
actionfield ("enable" | "disable" | "set_percentage") and optionalvaluefor set_percentage- This file: Single object with
valuefield that acceptsnumber | "enable" | "disable"Ensure this difference is intentional and that the conversion between these representations is handled correctly in the codebase.
There was a problem hiding this comment.
Additional Suggestions:
- The flag scheduler is started but never stopped during graceful shutdown, causing the interval to leak and continue running after the process shutdown.
View Details
📝 Patch Details
diff --git a/apps/api/src/index.ts b/apps/api/src/index.ts
index 2953b32e..d784c907 100644
--- a/apps/api/src/index.ts
+++ b/apps/api/src/index.ts
@@ -25,7 +25,7 @@ import { health } from "./routes/health";
import { publicApi } from "./routes/public";
import { query } from "./routes/query";
-import { startFlagScheduler } from "./services/flag-scheduler";
+import { startFlagScheduler, stopFlagScheduler } from "./services/flag-scheduler";
initTracing();
setupUncaughtErrorHandlers();
@@ -174,6 +174,7 @@ export default {
process.on("SIGINT", async () => {
logger.info("SIGINT received, shutting down gracefully...");
+ stopFlagScheduler();
await shutdownTracing().catch((error) =>
logger.error({ error }, "Shutdown error")
);
@@ -182,6 +183,7 @@ process.on("SIGINT", async () => {
process.on("SIGTERM", async () => {
logger.info("SIGTERM received, shutting down gracefully...");
+ stopFlagScheduler();
await shutdownTracing().catch((error) =>
logger.error({ error }, "Shutdown error")
);
Analysis
Missing stopFlagScheduler() call during graceful shutdown causes resource leak
What fails: The flag scheduler interval started by startFlagScheduler() is never stopped during graceful shutdown (SIGINT/SIGTERM), leaving the active timer running until the process is forcefully terminated.
How to reproduce:
- Start the API server: the flag scheduler interval is created on startup (line 32 of apps/api/src/index.ts)
- Send SIGINT or SIGTERM signal to the process (e.g., Ctrl+C or
kill -SIGTERM <pid>) - The shutdown handlers execute and call
process.exit(0)without stopping the scheduler - The
setIntervalcreated on line 13 of apps/api/src/services/flag-scheduler.ts remains active in memory
Result: Resource leak - the interval continues running (in memory) until hard process termination, and clean shutdown does not occur for this subsystem.
Expected: Per Node.js timers documentation, active timers keep the event loop running. Proper graceful shutdown requires calling stopFlagScheduler() (which already exists and calls clearInterval() on line 18 of flag-scheduler.ts) before exiting the process.
Fix applied: Added import of stopFlagScheduler from the flag-scheduler module and called it in both SIGINT and SIGTERM signal handlers before other cleanup operations.
View Details
📝 Patch Details
diff --git a/apps/api/src/routes/public/flags.ts b/apps/api/src/routes/public/flags.ts
index 79bd683b..070d50fa 100644
--- a/apps/api/src/routes/public/flags.ts
+++ b/apps/api/src/routes/public/flags.ts
@@ -18,6 +18,7 @@ const bulkFlagQuerySchema = t.Object({
userId: t.Optional(t.String()),
email: t.Optional(t.String()),
properties: t.Optional(t.String()),
+ environment: t.Optional(t.String()),
});
type UserContext = {
@@ -74,16 +75,21 @@ const getCachedFlag = cacheable(
);
const getCachedFlagsForClient = cacheable(
- (clientId: string) => {
+ (clientId: string, environment?: string) => {
const scopeCondition = or(
eq(flags.websiteId, clientId),
eq(flags.organizationId, clientId)
);
+ const environmentCondition = environment
+ ? eq(flags.environment, environment)
+ : isNull(flags.environment);
+
return db.query.flags.findMany({
where: and(
isNull(flags.deletedAt),
eq(flags.status, "active"),
+ environmentCondition,
scopeCondition
),
});
@@ -401,6 +407,7 @@ export const flagsRoute = new Elysia({ prefix: "/v1/flags" })
"flag.client_id": query.clientId || "missing",
"flag.has_user_id": Boolean(query.userId),
"flag.has_email": Boolean(query.email),
+ "flag.environment": query.environment || "missing",
});
try {
@@ -420,7 +427,7 @@ export const flagsRoute = new Elysia({ prefix: "/v1/flags" })
properties: parseProperties(query.properties),
};
- const allFlags = await getCachedFlagsForClient(query.clientId);
+ const allFlags = await getCachedFlagsForClient(query.clientId, query.environment);
setAttributes({
"flag.total_flags": allFlags.length,
Analysis
Bulk flags endpoint missing environment parameter support
What fails: The /v1/flags/bulk endpoint doesn't accept or filter by the environment parameter, while the individual /v1/flags/evaluate endpoint does, causing inconsistent behavior when multiple environments exist for the same client.
How to reproduce:
- Set up flags with the same key for different environments (e.g.,
feature-flagwithenvironment=stagingandenvironment=production) - Call
/v1/flags/evaluate?key=feature-flag&clientId=test-client&environment=staging- returns only the staging flag - Call
/v1/flags/bulk?clientId=test-client&environment=staging- returns all active flags for the client, including flags from other environments
Result: The bulk endpoint ignores the environment parameter and returns all flags regardless of environment filtering, while the individual endpoint correctly filters by environment.
Expected: Both endpoints should respect the environment parameter equally. When an environment is specified, only flags matching that environment (or with NULL environment) should be returned.
What changed:
- Added
environment: t.Optional(t.String())tobulkFlagQuerySchemato accept the parameter - Updated
getCachedFlagsForClient()to accept and filter by environment, using the same logic asgetCachedFlag() - Pass
query.environmenttogetCachedFlagsForClient()call in the bulk endpoint - Added environment attribute to trace logging for consistency with the individual endpoint
Design note: The database schema allows multiple flags with the same key in different environments (environment is not part of unique constraints but is used as a filtering dimension). Both endpoints now consistently support this multi-environment design.
* feat: make scroll down button look more visible * fix(agent): chat status and status indicator
* chore: fix status type * feat(agent): make commands menu fully accessible
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (15)
apps/dashboard/app/(main)/websites/[id]/flags/_components/flag-examples.tsx (5)
22-27: Replaceanytype with a proper union type.The
valuefield usesany, which violates coding guidelines. Define a proper union type based on expected variant values.variants: Array<{ key: string; - value: any; + value: string | number | boolean | Record<string, unknown>; weight: number; description: string; }>;
215-220: Remove unused props from interface.
maxExamples,variant, andshowExamplesare declared but never used in the component.interface FlagExamplesProps { onCreateFromTemplate?: (template: FlagTemplate) => void; - maxExamples?: number; - variant?: string; - showExamples?: boolean; + websiteId: string; }
227-239: HardcodedwebsiteIdand missinguserIdin query keys cause stale cache issues.
- The websiteId
"h3mH7S0t8_MIqPvjLeYCb"is hardcoded but this component is within a[id]route—pass it via props or route params.- When
userIdchanges viahandleRefetchAsNewUser, React Query won't refetch becauseuserIdisn't in the query key.export function FlagExamples({ onCreateFromTemplate, + websiteId, }: FlagExamplesProps) { const [userId, setUserId] = useState("1aZtjWs4U4vQMa3Z2j5XA5PDy76fXNGE"); const { data, refetch, isFetching } = useQuery({ - queryKey: ["examples-display-strategy"], + queryKey: ["examples-display-strategy", websiteId, userId], queryFn: async () => { - console.log("Fetching examples display strategy"); - return await getExamplesDisplayStrategy("h3mH7S0t8_MIqPvjLeYCb", userId, "test") + return await getExamplesDisplayStrategy(websiteId, userId, "test") } }); const { data: shouldShowExamples, refetch: refetchShouldShowExamples, isFetching: isFetchingShouldShowExamples } = useQuery({ - queryKey: ["should-show-examples"], + queryKey: ["should-show-examples", websiteId, userId], queryFn: async () => { - return await getShouldShowExamples("h3mH7S0t8_MIqPvjLeYCb", userId, "test") + return await getShouldShowExamples(websiteId, userId, "test") } });
243-252: State update race condition prevents new user refetch.
setUserId(newUserId)followed immediately byrefetch()causes the query to use the staleuserIdbecause React state updates are asynchronous. SinceuserIdis now in the query key (per the fix above), React Query will automatically refetch whenuserIdchanges—remove the manual refetch calls.const handleRefetchAsNewUser = (isRefetchShouldShowExamples?: boolean) => { const newUserId = `test-user-${Math.random().toString(36).substring(2, 15)}`; - console.log(`🔄 Switching to new user: ${newUserId}`); setUserId(newUserId); - if (isRefetchShouldShowExamples) { - refetchShouldShowExamples(); - } else { - refetch(); - } + // React Query will automatically refetch when userId changes in queryKey };
230-230: Remove debugconsole.logstatement.Debug logging should be removed before merging.
apps/dashboard/app/(main)/websites/[id]/flags/_components/flag-sheet.tsx (5)
178-196: Missingformin useEffect dependency array.The
useEffectusesform.setValuebutformis not in the dependency array. WhileformfromuseFormis typically stable, the linter may flag this.- }, [watchedName, keyManuallyEdited, isEditing, watchedIsScheduleEnabled]); + }, [watchedName, keyManuallyEdited, isEditing, watchedIsScheduleEnabled, form, flag?.id]);
216-237: Replaceanytype with proper mutation input types.Using
anybypasses TypeScript's type checking and violates coding guidelines. Define proper types for the mutation data.+ type FlagMutationData = { + name: string; + description: string; + type: string; + status: string; + defaultValue: boolean; + rolloutPercentage: number; + rules: Array<Record<string, unknown>>; + variants: Array<Record<string, unknown>>; + dependencies: string[]; + environment: string | null; + id?: string; + websiteId?: string; + key?: string; + }; + - const mutationData: any = { + const mutationData: FlagMutationData = { name: data.name, // ... rest of fields } - const updatedFlag = await mutation.mutateAsync(mutationData as any); + const updatedFlag = await mutation.mutateAsync(mutationData);
242-254: Replaceanytype for schedule mutation data.Same issue—
scheduleMutationDatausesanytype.- let scheduleMutationData: any = { + const scheduleMutationData = { flagId: flagIdToUse, type: scheduleData.type, scheduledAt: scheduleData.scheduledAt, rolloutSteps: scheduleData.rolloutSteps || [], isEnabled: scheduleData.isEnabled, - }; + } as const; + if (schedule && schedule.id) { - scheduleMutationData.id = schedule.id; - await updateFlagScheduleMutation.mutateAsync(scheduleMutationData); + await updateFlagScheduleMutation.mutateAsync({ ...scheduleMutationData, id: schedule.id }); } else { await createFlagScheduleMutation.mutateAsync(scheduleMutationData); }
740-746: Avoid direct mutation of form state array.The code mutates
watchedRolloutStepsdirectly (newSteps[idx].scheduledAt = ...), which can cause React state inconsistencies and missed re-renders. Create a new array with immutable updates.onSelect={(date) => { if (date) { ensureScheduleFields(); - const newSteps = [...(watchedRolloutSteps || [])]; - newSteps[idx].scheduledAt = date.toISOString(); + const newSteps = (watchedRolloutSteps || []).map((s, i) => + i === idx ? { ...s, scheduledAt: date.toISOString() } : s + ); form.setValue("schedule.rolloutSteps", newSteps); } }}Apply the same immutable pattern at lines 767-769 and 786-788.
749-771: Time input format mismatch.The
<input type="time">expectsHH:MMformat (24-hour), butDATE_FORMATS.DATE_TIME_12Hlikely produces a full date-time string like "Jan 15, 2025 03:30 PM", causing the input to not display correctly.<div className="p-3 border-t"> <Input type="time" defaultValue={ step.scheduledAt - ? formatDate( - new Date(step.scheduledAt), - DATE_FORMATS.DATE_TIME_12H - ) + ? new Date(step.scheduledAt) + .toTimeString() + .slice(0, 5) : "" } onChange={(e) => {packages/db/src/drizzle/schema.ts (1)
668-675: Type mismatch remains:RolloutSteptype vsflagSchedules.rolloutStepscolumn.The exported
RolloutSteptype uses anactionfield discriminator, but therolloutStepsjsonb column (lines 760-762) uses avaluefield with union typenumber | "enable" | "disable". This inconsistency was flagged previously and should be resolved for type safety.Additionally, there's a duplicate
RolloutStepexport inpackages/shared/src/flags/index.tswhich uses a different schema-based type. Consider consolidating to a single source of truth.Align the types by either:
- Update the table to use the
RolloutSteptype:- rolloutSteps: jsonb("rollout_steps").$type< - { scheduledAt: string; value: number | "enable" | "disable", executedAt?: string }[] - >(), + rolloutSteps: jsonb("rollout_steps").$type<RolloutStep[]>(),
- Or update
RolloutStepto match the actual column shape and remove the shared package duplicate.apps/api/src/services/flag-scheduler.ts (1)
60-76: RedundantDateinstantiation and rollout step logic.Line 62 creates two separate
Dateobjects. Additionally, the logic on line 64 marks steps as executed ifscheduledAt <= now, which correctly prevents replay of older steps (addressing a past concern), but the conditionstep.executedAt || new Date(step.scheduledAt) <= nowwill mark steps that are already executed OR past/current—this seems intentional for the catch-up scenario.Minor optimization to avoid redundant Date instantiation:
const now = new Date(); - const nowIso = new Date().toISOString(); + const nowIso = now.toISOString();packages/rpc/src/routers/flag-schedules.ts (3)
56-60: Return null instead of throwing error when flag has no schedules.Throwing
NOT_FOUNDwhen a valid flag exists but has no schedules breaks frontend form initialization. The frontend expectsnullorundefinedfor empty schedule state.Apply this fix:
- if (!schedules[0]) { - throw new ORPCError("NOT_FOUND", { message: "No schedules found for this flag" }); - } - - return schedules[0]; + return schedules[0] || null;
104-107: Usenullinstead ofundefinedfor database fieldexecutedAt.Setting
executedAt: undefined(line 106) may behave inconsistently across databases. Usenullexplicitly for clarity and consistency with the update handler.Apply this diff:
rolloutSteps: input.rolloutSteps?.map((step) => ({ ...step, - executedAt: undefined, + executedAt: null, })),
180-184: Usenullinstead ofundefinedfor database fieldexecutedAt.Setting
executedAt: undefined(line 183) may behave inconsistently across databases. Usenullexplicitly to match the create handler and database expectations.Apply this diff:
rolloutSteps: updates.rolloutSteps?.map((step) => ({ value: step.value, scheduledAt: step.scheduledAt, - executedAt: undefined, + executedAt: null, })),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
apps/api/src/index.ts(2 hunks)apps/api/src/routes/webhooks/flag-scheduler.ts(1 hunks)apps/api/src/services/flag-scheduler.ts(1 hunks)apps/dashboard/app/(main)/websites/[id]/flags/_components/flag-examples.tsx(1 hunks)apps/dashboard/app/(main)/websites/[id]/flags/_components/flag-sheet.tsx(1 hunks)apps/dashboard/components/providers/billing-provider.tsx(1 hunks)packages/db/src/drizzle/schema.ts(3 hunks)packages/rpc/src/routers/flag-schedules.ts(1 hunks)packages/rpc/src/services/flag-scheduler.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (12)
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit inference engine (.cursor/rules/guidelines.mdc)
**/*.{ts,tsx,js,jsx,vue}: Never block paste in<input>or<textarea>elements
Enter submits focused text input; in<textarea>, ⌘/Ctrl+Enter submits; Enter adds newline
Compatible with password managers and 2FA; allow pasting one-time codes
Files:
apps/api/src/routes/webhooks/flag-scheduler.tsapps/api/src/index.tspackages/db/src/drizzle/schema.tsapps/dashboard/components/providers/billing-provider.tsxapps/api/src/services/flag-scheduler.tspackages/rpc/src/services/flag-scheduler.tspackages/rpc/src/routers/flag-schedules.tsapps/dashboard/app/(main)/websites/[id]/flags/_components/flag-sheet.tsxapps/dashboard/app/(main)/websites/[id]/flags/_components/flag-examples.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/guidelines.mdc)
**/*.{ts,tsx,js,jsx}: Trim values to handle text expansion and trailing spaces in form submissions
URL reflects state (deep-link filters/tabs/pagination/expanded panels); prefer libraries likenuqs
Back/Forward buttons restore scroll position
Delay first tooltip in a group; subsequent peers have no delay
Use locale-aware formatting for dates, times, numbers, and currency
Batch layout reads/writes; avoid unnecessary reflows/repaints
Virtualize large lists using libraries likevirtua
**/*.{ts,tsx,js,jsx}: Don't useaccessKeyattribute on any HTML element.
Don't setaria-hidden="true"on focusable elements.
Don't add ARIA roles, states, and properties to elements that don't support them.
Don't use distracting elements like<marquee>or<blink>.
Only use thescopeprop on<th>elements.
Don't assign non-interactive ARIA roles to interactive HTML elements.
Make sure label elements have text content and are associated with an input.
Don't assign interactive ARIA roles to non-interactive HTML elements.
Don't assigntabIndexto non-interactive HTML elements.
Don't use positive integers fortabIndexproperty.
Don't include "image", "picture", or "photo" in img alt prop.
Don't use explicit role property that's the same as the implicit/default role.
Make static elements with click handlers use a valid role attribute.
Always include atitleelement for SVG elements.
Give all elements requiring alt text meaningful information for screen readers.
Make sure anchors have content that's accessible to screen readers.
AssigntabIndexto non-interactive HTML elements witharia-activedescendant.
Include all required ARIA attributes for elements with ARIA roles.
Make sure ARIA properties are valid for the element's supported roles.
Always include atypeattribute for button elements.
Make elements with interactive roles and handlers focusable.
Give heading elements content that's accessible to screen readers (not hidden witharia-hidden).
Always include...
Files:
apps/api/src/routes/webhooks/flag-scheduler.tsapps/api/src/index.tspackages/db/src/drizzle/schema.tsapps/dashboard/components/providers/billing-provider.tsxapps/api/src/services/flag-scheduler.tspackages/rpc/src/services/flag-scheduler.tspackages/rpc/src/routers/flag-schedules.tsapps/dashboard/app/(main)/websites/[id]/flags/_components/flag-sheet.tsxapps/dashboard/app/(main)/websites/[id]/flags/_components/flag-examples.tsx
**/*.{ts,tsx,js,jsx,css,scss,sass,less}
📄 CodeRabbit inference engine (.cursor/rules/guidelines.mdc)
**/*.{ts,tsx,js,jsx,css,scss,sass,less}: During drag operations, disable text selection and setinerton dragged element and containers
Animations must be interruptible and input-driven; avoid autoplay
Files:
apps/api/src/routes/webhooks/flag-scheduler.tsapps/api/src/index.tspackages/db/src/drizzle/schema.tsapps/dashboard/components/providers/billing-provider.tsxapps/api/src/services/flag-scheduler.tspackages/rpc/src/services/flag-scheduler.tspackages/rpc/src/routers/flag-schedules.tsapps/dashboard/app/(main)/websites/[id]/flags/_components/flag-sheet.tsxapps/dashboard/app/(main)/websites/[id]/flags/_components/flag-examples.tsx
**/*.{ts,tsx,jsx}
📄 CodeRabbit inference engine (.cursor/rules/ultracite.mdc)
**/*.{ts,tsx,jsx}: Use semantic elements instead of role attributes in JSX.
Don't use unnecessary fragments.
Don't pass children as props.
Don't use the return value of React.render.
Make sure all dependencies are correctly specified in React hooks.
Make sure all React hooks are called from the top level of component functions.
Don't forget key props in iterators and collection literals.
Don't destructure props inside JSX components in Solid projects.
Don't define React components inside other components.
Don't use event handlers on non-interactive elements.
Don't assign to React component props.
Don't use bothchildrenanddangerouslySetInnerHTMLprops on the same element.
Don't use dangerous JSX props.
Don't use Array index in keys.
Don't insert comments as text nodes.
Don't assign JSX properties multiple times.
Don't add extra closing tags for components without children.
Use<>...</>instead of<Fragment>...</Fragment>.
Watch out for possible "wrong" semicolons inside JSX elements.
Make sure void (self-closing) elements don't have children.
Don't usetarget="_blank"withoutrel="noopener".
Don't use<img>elements in Next.js projects.
Don't use<head>elements in Next.js projects.
Files:
apps/api/src/routes/webhooks/flag-scheduler.tsapps/api/src/index.tspackages/db/src/drizzle/schema.tsapps/dashboard/components/providers/billing-provider.tsxapps/api/src/services/flag-scheduler.tspackages/rpc/src/services/flag-scheduler.tspackages/rpc/src/routers/flag-schedules.tsapps/dashboard/app/(main)/websites/[id]/flags/_components/flag-sheet.tsxapps/dashboard/app/(main)/websites/[id]/flags/_components/flag-examples.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/ultracite.mdc)
**/*.{ts,tsx}: Don't use primitive type aliases or misleading types.
Don't use empty type parameters in type aliases and interfaces.
Don't use any or unknown as type constraints.
Don't return a value from a function with the return type 'void'.
Don't use the TypeScript directive @ts-ignore.
Don't use TypeScript enums.
Don't add type annotations to variables, parameters, and class properties that are initialized with literal expressions.
Don't use TypeScript namespaces.
Don't use non-null assertions with the!postfix operator.
Don't use parameter properties in class constructors.
Don't use user-defined types.
Useas constinstead of literal types and type annotations.
Use eitherT[]orArray<T>consistently.
Initialize each enum member value explicitly.
Useexport typefor types.
Useimport typefor types.
Make sure all enum members are literal values.
Don't use TypeScript const enum.
Don't declare empty interfaces.
Don't let variables evolve into any type through reassignments.
Don't use the any type.
Don't misuse the non-null assertion operator (!) in TypeScript files.
Don't use implicit any type on variable declarations.
Don't merge interfaces and classes unsafely.
Don't use overload signatures that aren't next to each other.
Use the namespace keyword instead of the module keyword to declare TypeScript namespaces.
Use consistent accessibility modifiers on class properties and methods.
Use function types instead of object types with call signatures.
Don't use void type outside of generic or return types.Do not use types 'any', 'unknown' or 'never', use proper explicit types
Files:
apps/api/src/routes/webhooks/flag-scheduler.tsapps/api/src/index.tspackages/db/src/drizzle/schema.tsapps/dashboard/components/providers/billing-provider.tsxapps/api/src/services/flag-scheduler.tspackages/rpc/src/services/flag-scheduler.tspackages/rpc/src/routers/flag-schedules.tsapps/dashboard/app/(main)/websites/[id]/flags/_components/flag-sheet.tsxapps/dashboard/app/(main)/websites/[id]/flags/_components/flag-examples.tsx
!(**/pages/_document.{ts,tsx,jsx})**/*.{ts,tsx,jsx}
📄 CodeRabbit inference engine (.cursor/rules/ultracite.mdc)
Don't import next/document outside of pages/_document.jsx in Next.js projects.
Files:
apps/api/src/routes/webhooks/flag-scheduler.tsapps/api/src/index.tspackages/db/src/drizzle/schema.tsapps/dashboard/components/providers/billing-provider.tsxapps/api/src/services/flag-scheduler.tspackages/rpc/src/services/flag-scheduler.tspackages/rpc/src/routers/flag-schedules.tsapps/dashboard/app/(main)/websites/[id]/flags/_components/flag-sheet.tsxapps/dashboard/app/(main)/websites/[id]/flags/_components/flag-examples.tsx
**/*.{ts,tsx,html,css}
📄 CodeRabbit inference engine (.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc)
Use
bun build <file.html|file.ts|file.css>instead ofwebpackoresbuildfor bundling
Files:
apps/api/src/routes/webhooks/flag-scheduler.tsapps/api/src/index.tspackages/db/src/drizzle/schema.tsapps/dashboard/components/providers/billing-provider.tsxapps/api/src/services/flag-scheduler.tspackages/rpc/src/services/flag-scheduler.tspackages/rpc/src/routers/flag-schedules.tsapps/dashboard/app/(main)/websites/[id]/flags/_components/flag-sheet.tsxapps/dashboard/app/(main)/websites/[id]/flags/_components/flag-examples.tsx
**/*.{html,htm,tsx,jsx,vue}
📄 CodeRabbit inference engine (.cursor/rules/guidelines.mdc)
**/*.{html,htm,tsx,jsx,vue}: Useautocompleteattribute with meaningfulname; use correcttypeandinputmode
Disable spellcheck for email/code/username inputs usingspellcheck="false"
Links are<a>or<Link>components for navigation; support Cmd/Ctrl/middle-click
Use politearia-livefor toasts and inline validation feedback
Autofocus on desktop when there's a single primary input; rarely on mobile to avoid layout shift
<title>tag must match current context
Use redundant status cues (not color-only); icon-only elements have text labels
Use accurate accessible names viaaria-label; mark decorative elements witharia-hidden; verify in Accessibility Tree
Icon-only buttons must have descriptivearia-label
Prefer native semantics (button,a,label,table) before ARIA
Use non-breaking spaces to glue terms:10 MB,⌘ + K,Vercel SDK
Preload only above-the-fold images; lazy-load the rest
Files:
apps/dashboard/components/providers/billing-provider.tsxapps/dashboard/app/(main)/websites/[id]/flags/_components/flag-sheet.tsxapps/dashboard/app/(main)/websites/[id]/flags/_components/flag-examples.tsx
**/*.{css,scss,sass,less,html,htm,tsx,jsx,vue}
📄 CodeRabbit inference engine (.cursor/rules/guidelines.mdc)
**/*.{css,scss,sass,less,html,htm,tsx,jsx,vue}: No dead zones on checkboxes/radios; label and control share one generous hit target
Usescroll-margin-topon headings for anchored links; include 'Skip to content' link; use hierarchical<h1>–<h6>
Prevent Cumulative Layout Shift (CLS) from images by using explicit dimensions or reserved space
Files:
apps/dashboard/components/providers/billing-provider.tsxapps/dashboard/app/(main)/websites/[id]/flags/_components/flag-sheet.tsxapps/dashboard/app/(main)/websites/[id]/flags/_components/flag-examples.tsx
**/*.{tsx,jsx,vue}
📄 CodeRabbit inference engine (.cursor/rules/guidelines.mdc)
Don't ship the schema—visuals may omit labels but accessible names must still exist
Files:
apps/dashboard/components/providers/billing-provider.tsxapps/dashboard/app/(main)/websites/[id]/flags/_components/flag-sheet.tsxapps/dashboard/app/(main)/websites/[id]/flags/_components/flag-examples.tsx
**/*.{tsx,jsx}
📄 CodeRabbit inference engine (.cursor/rules/guidelines.mdc)
Prefer uncontrolled inputs; make controlled loops cheap (optimize keystroke cost)
**/*.{tsx,jsx}: Don't use lucide for icons, only use phosphor icons, use width='duotone' for most, but for arrows use fill, for plus icons don't add width
Always use error boundaries properly
Use Icon at the end of phosphor react icons, like CaretIcon not Caret, and that's the default import, not as
Almost never use useEffect unless it's critical
Files:
apps/dashboard/components/providers/billing-provider.tsxapps/dashboard/app/(main)/websites/[id]/flags/_components/flag-sheet.tsxapps/dashboard/app/(main)/websites/[id]/flags/_components/flag-examples.tsx
**/*.{tsx,jsx,html}
📄 CodeRabbit inference engine (.cursor/rules/01-MUST-DO.mdc)
**/*.{tsx,jsx,html}: When using 'text-right', always add 'text-balance' so it's not ugly
Always use 'rounded', not 'rounded-xl' or 'rounded-md'
Files:
apps/dashboard/components/providers/billing-provider.tsxapps/dashboard/app/(main)/websites/[id]/flags/_components/flag-sheet.tsxapps/dashboard/app/(main)/websites/[id]/flags/_components/flag-examples.tsx
🧠 Learnings (35)
📚 Learning: 2025-12-02T22:57:17.246Z
Learnt from: CR
Repo: databuddy-analytics/Databuddy PR: 0
File: .cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc:0-0
Timestamp: 2025-12-02T22:57:17.246Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Use `Bun.serve()` with routes and WebSocket support instead of `express`
Applied to files:
apps/api/src/index.ts
📚 Learning: 2025-12-02T22:57:07.791Z
Learnt from: CR
Repo: databuddy-analytics/Databuddy PR: 0
File: .cursor/rules/ultracite.mdc:0-0
Timestamp: 2025-12-02T22:57:07.791Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Don't have unused imports.
Applied to files:
packages/db/src/drizzle/schema.tsapps/dashboard/app/(main)/websites/[id]/flags/_components/flag-sheet.tsx
📚 Learning: 2025-12-02T22:57:07.791Z
Learnt from: CR
Repo: databuddy-analytics/Databuddy PR: 0
File: .cursor/rules/ultracite.mdc:0-0
Timestamp: 2025-12-02T22:57:07.791Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Don't use constants whose value is the upper-case version of their name.
Applied to files:
packages/db/src/drizzle/schema.ts
📚 Learning: 2025-12-02T22:57:07.791Z
Learnt from: CR
Repo: databuddy-analytics/Databuddy PR: 0
File: .cursor/rules/ultracite.mdc:0-0
Timestamp: 2025-12-02T22:57:07.791Z
Learning: Applies to **/*.{ts,tsx} : Make sure all enum members are literal values.
Applied to files:
packages/db/src/drizzle/schema.ts
📚 Learning: 2025-12-02T22:57:07.791Z
Learnt from: CR
Repo: databuddy-analytics/Databuddy PR: 0
File: .cursor/rules/ultracite.mdc:0-0
Timestamp: 2025-12-02T22:57:07.791Z
Learning: Applies to **/*.{ts,tsx} : Don't use TypeScript const enum.
Applied to files:
packages/db/src/drizzle/schema.ts
📚 Learning: 2025-12-02T22:57:07.791Z
Learnt from: CR
Repo: databuddy-analytics/Databuddy PR: 0
File: .cursor/rules/ultracite.mdc:0-0
Timestamp: 2025-12-02T22:57:07.791Z
Learning: Applies to **/*.{ts,tsx} : Don't use TypeScript enums.
Applied to files:
packages/db/src/drizzle/schema.ts
📚 Learning: 2025-12-02T22:57:07.791Z
Learnt from: CR
Repo: databuddy-analytics/Databuddy PR: 0
File: .cursor/rules/ultracite.mdc:0-0
Timestamp: 2025-12-02T22:57:07.791Z
Learning: Applies to **/*.{ts,tsx} : Don't use primitive type aliases or misleading types.
Applied to files:
packages/db/src/drizzle/schema.tsapps/api/src/services/flag-scheduler.tspackages/rpc/src/routers/flag-schedules.tsapps/dashboard/app/(main)/websites/[id]/flags/_components/flag-sheet.tsxapps/dashboard/app/(main)/websites/[id]/flags/_components/flag-examples.tsx
📚 Learning: 2025-12-02T22:57:07.791Z
Learnt from: CR
Repo: databuddy-analytics/Databuddy PR: 0
File: .cursor/rules/ultracite.mdc:0-0
Timestamp: 2025-12-02T22:57:07.791Z
Learning: Applies to **/*.{ts,tsx} : Don't use the any type.
Applied to files:
apps/api/src/services/flag-scheduler.tspackages/rpc/src/routers/flag-schedules.tsapps/dashboard/app/(main)/websites/[id]/flags/_components/flag-sheet.tsxapps/dashboard/app/(main)/websites/[id]/flags/_components/flag-examples.tsx
📚 Learning: 2025-12-02T22:57:07.791Z
Learnt from: CR
Repo: databuddy-analytics/Databuddy PR: 0
File: .cursor/rules/ultracite.mdc:0-0
Timestamp: 2025-12-02T22:57:07.791Z
Learning: Applies to **/*.{ts,tsx} : Don't use implicit any type on variable declarations.
Applied to files:
apps/api/src/services/flag-scheduler.tspackages/rpc/src/routers/flag-schedules.tsapps/dashboard/app/(main)/websites/[id]/flags/_components/flag-sheet.tsxapps/dashboard/app/(main)/websites/[id]/flags/_components/flag-examples.tsx
📚 Learning: 2025-12-02T22:57:07.791Z
Learnt from: CR
Repo: databuddy-analytics/Databuddy PR: 0
File: .cursor/rules/ultracite.mdc:0-0
Timestamp: 2025-12-02T22:57:07.791Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Don't use explicit role property that's the same as the implicit/default role.
Applied to files:
apps/api/src/services/flag-scheduler.tsapps/dashboard/app/(main)/websites/[id]/flags/_components/flag-sheet.tsx
📚 Learning: 2025-12-02T22:57:07.791Z
Learnt from: CR
Repo: databuddy-analytics/Databuddy PR: 0
File: .cursor/rules/ultracite.mdc:0-0
Timestamp: 2025-12-02T22:57:07.791Z
Learning: Applies to **/*.{ts,tsx} : Use either `T[]` or `Array<T>` consistently.
Applied to files:
apps/api/src/services/flag-scheduler.tsapps/dashboard/app/(main)/websites/[id]/flags/_components/flag-sheet.tsxapps/dashboard/app/(main)/websites/[id]/flags/_components/flag-examples.tsx
📚 Learning: 2025-12-05T15:04:07.417Z
Learnt from: CR
Repo: databuddy-analytics/Databuddy PR: 0
File: .cursor/rules/01-MUST-DO.mdc:0-0
Timestamp: 2025-12-05T15:04:07.417Z
Learning: Applies to **/*.{ts,tsx} : Do not use types 'any', 'unknown' or 'never', use proper explicit types
Applied to files:
apps/api/src/services/flag-scheduler.tsapps/dashboard/app/(main)/websites/[id]/flags/_components/flag-sheet.tsxapps/dashboard/app/(main)/websites/[id]/flags/_components/flag-examples.tsx
📚 Learning: 2025-12-02T22:57:07.791Z
Learnt from: CR
Repo: databuddy-analytics/Databuddy PR: 0
File: .cursor/rules/ultracite.mdc:0-0
Timestamp: 2025-12-02T22:57:07.791Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Use Date.now() to get milliseconds since the Unix Epoch.
Applied to files:
apps/api/src/services/flag-scheduler.ts
📚 Learning: 2025-12-02T22:57:07.791Z
Learnt from: CR
Repo: databuddy-analytics/Databuddy PR: 0
File: .cursor/rules/ultracite.mdc:0-0
Timestamp: 2025-12-02T22:57:07.791Z
Learning: Applies to **/*.{ts,tsx,jsx} : Make sure all dependencies are correctly specified in React hooks.
Applied to files:
apps/dashboard/app/(main)/websites/[id]/flags/_components/flag-sheet.tsx
📚 Learning: 2025-12-05T15:04:07.417Z
Learnt from: CR
Repo: databuddy-analytics/Databuddy PR: 0
File: .cursor/rules/01-MUST-DO.mdc:0-0
Timestamp: 2025-12-05T15:04:07.417Z
Learning: Applies to **/*.{tsx,jsx} : Almost never use useEffect unless it's critical
Applied to files:
apps/dashboard/app/(main)/websites/[id]/flags/_components/flag-sheet.tsx
📚 Learning: 2025-12-02T22:57:07.791Z
Learnt from: CR
Repo: databuddy-analytics/Databuddy PR: 0
File: .cursor/rules/ultracite.mdc:0-0
Timestamp: 2025-12-02T22:57:07.791Z
Learning: Applies to **/*.{ts,tsx} : Don't misuse the non-null assertion operator (!) in TypeScript files.
Applied to files:
apps/dashboard/app/(main)/websites/[id]/flags/_components/flag-sheet.tsx
📚 Learning: 2025-12-02T22:57:07.791Z
Learnt from: CR
Repo: databuddy-analytics/Databuddy PR: 0
File: .cursor/rules/ultracite.mdc:0-0
Timestamp: 2025-12-02T22:57:07.791Z
Learning: Applies to **/*.{ts,tsx} : Use `as const` instead of literal types and type annotations.
Applied to files:
apps/dashboard/app/(main)/websites/[id]/flags/_components/flag-sheet.tsxapps/dashboard/app/(main)/websites/[id]/flags/_components/flag-examples.tsx
📚 Learning: 2025-12-02T22:57:07.791Z
Learnt from: CR
Repo: databuddy-analytics/Databuddy PR: 0
File: .cursor/rules/ultracite.mdc:0-0
Timestamp: 2025-12-02T22:57:07.791Z
Learning: Applies to **/*.{ts,tsx} : Don't use any or unknown as type constraints.
Applied to files:
apps/dashboard/app/(main)/websites/[id]/flags/_components/flag-sheet.tsxapps/dashboard/app/(main)/websites/[id]/flags/_components/flag-examples.tsx
📚 Learning: 2025-12-02T22:57:07.791Z
Learnt from: CR
Repo: databuddy-analytics/Databuddy PR: 0
File: .cursor/rules/ultracite.mdc:0-0
Timestamp: 2025-12-02T22:57:07.791Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Don't use unnecessary boolean casts.
Applied to files:
apps/dashboard/app/(main)/websites/[id]/flags/_components/flag-sheet.tsxapps/dashboard/app/(main)/websites/[id]/flags/_components/flag-examples.tsx
📚 Learning: 2025-12-02T22:57:07.791Z
Learnt from: CR
Repo: databuddy-analytics/Databuddy PR: 0
File: .cursor/rules/ultracite.mdc:0-0
Timestamp: 2025-12-02T22:57:07.791Z
Learning: Applies to **/*.{ts,tsx} : Don't use user-defined types.
Applied to files:
apps/dashboard/app/(main)/websites/[id]/flags/_components/flag-sheet.tsx
📚 Learning: 2025-12-05T15:04:07.417Z
Learnt from: CR
Repo: databuddy-analytics/Databuddy PR: 0
File: .cursor/rules/01-MUST-DO.mdc:0-0
Timestamp: 2025-12-05T15:04:07.417Z
Learning: Applies to **/*.{tsx,jsx} : Use Icon at the end of phosphor react icons, like CaretIcon not Caret, and that's the default import, not as
Applied to files:
apps/dashboard/app/(main)/websites/[id]/flags/_components/flag-sheet.tsx
📚 Learning: 2025-12-05T15:04:07.417Z
Learnt from: CR
Repo: databuddy-analytics/Databuddy PR: 0
File: .cursor/rules/01-MUST-DO.mdc:0-0
Timestamp: 2025-12-05T15:04:07.417Z
Learning: Applies to **/*.{tsx,jsx} : Don't use lucide for icons, only use phosphor icons, use width='duotone' for most, but for arrows use fill, for plus icons don't add width
Applied to files:
apps/dashboard/app/(main)/websites/[id]/flags/_components/flag-sheet.tsx
📚 Learning: 2025-12-02T22:57:07.791Z
Learnt from: CR
Repo: databuddy-analytics/Databuddy PR: 0
File: .cursor/rules/ultracite.mdc:0-0
Timestamp: 2025-12-02T22:57:07.791Z
Learning: Applies to **/*.{ts,tsx} : Use function types instead of object types with call signatures.
Applied to files:
apps/dashboard/app/(main)/websites/[id]/flags/_components/flag-sheet.tsx
📚 Learning: 2025-12-02T22:57:07.791Z
Learnt from: CR
Repo: databuddy-analytics/Databuddy PR: 0
File: .cursor/rules/ultracite.mdc:0-0
Timestamp: 2025-12-02T22:57:07.791Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Make sure iterable callbacks return consistent values.
Applied to files:
apps/dashboard/app/(main)/websites/[id]/flags/_components/flag-sheet.tsx
📚 Learning: 2025-12-02T22:55:00.415Z
Learnt from: CR
Repo: databuddy-analytics/Databuddy PR: 0
File: .cursor/rules/guidelines.mdc:0-0
Timestamp: 2025-12-02T22:55:00.415Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Use locale-aware formatting for dates, times, numbers, and currency
Applied to files:
apps/dashboard/app/(main)/websites/[id]/flags/_components/flag-sheet.tsx
📚 Learning: 2025-12-02T22:57:07.791Z
Learnt from: CR
Repo: databuddy-analytics/Databuddy PR: 0
File: .cursor/rules/ultracite.mdc:0-0
Timestamp: 2025-12-02T22:57:07.791Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Don't have unused function parameters.
Applied to files:
apps/dashboard/app/(main)/websites/[id]/flags/_components/flag-sheet.tsx
📚 Learning: 2025-12-02T22:57:07.791Z
Learnt from: CR
Repo: databuddy-analytics/Databuddy PR: 0
File: .cursor/rules/ultracite.mdc:0-0
Timestamp: 2025-12-02T22:57:07.791Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Don't use unnecessary labels.
Applied to files:
apps/dashboard/app/(main)/websites/[id]/flags/_components/flag-sheet.tsx
📚 Learning: 2025-12-02T22:57:07.791Z
Learnt from: CR
Repo: databuddy-analytics/Databuddy PR: 0
File: .cursor/rules/ultracite.mdc:0-0
Timestamp: 2025-12-02T22:57:07.791Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Don't have unused labels.
Applied to files:
apps/dashboard/app/(main)/websites/[id]/flags/_components/flag-sheet.tsx
📚 Learning: 2025-12-02T22:57:07.791Z
Learnt from: CR
Repo: databuddy-analytics/Databuddy PR: 0
File: .cursor/rules/ultracite.mdc:0-0
Timestamp: 2025-12-02T22:57:07.791Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Don't use useless undefined.
Applied to files:
apps/dashboard/app/(main)/websites/[id]/flags/_components/flag-sheet.tsx
📚 Learning: 2025-12-02T22:57:07.791Z
Learnt from: CR
Repo: databuddy-analytics/Databuddy PR: 0
File: .cursor/rules/ultracite.mdc:0-0
Timestamp: 2025-12-02T22:57:07.791Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Don't have unused variables.
Applied to files:
apps/dashboard/app/(main)/websites/[id]/flags/_components/flag-sheet.tsx
📚 Learning: 2025-12-02T22:57:07.791Z
Learnt from: CR
Repo: databuddy-analytics/Databuddy PR: 0
File: .cursor/rules/ultracite.mdc:0-0
Timestamp: 2025-12-02T22:57:07.791Z
Learning: Applies to **/*.{ts,tsx} : Don't use the TypeScript directive ts-ignore.
Applied to files:
apps/dashboard/app/(main)/websites/[id]/flags/_components/flag-sheet.tsx
📚 Learning: 2025-12-02T22:57:07.791Z
Learnt from: CR
Repo: databuddy-analytics/Databuddy PR: 0
File: .cursor/rules/ultracite.mdc:0-0
Timestamp: 2025-12-02T22:57:07.791Z
Learning: Applies to **/*.{ts,tsx} : Don't add type annotations to variables, parameters, and class properties that are initialized with literal expressions.
Applied to files:
apps/dashboard/app/(main)/websites/[id]/flags/_components/flag-sheet.tsx
📚 Learning: 2025-12-02T22:57:07.791Z
Learnt from: CR
Repo: databuddy-analytics/Databuddy PR: 0
File: .cursor/rules/ultracite.mdc:0-0
Timestamp: 2025-12-02T22:57:07.791Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Make sure label elements have text content and are associated with an input.
Applied to files:
apps/dashboard/app/(main)/websites/[id]/flags/_components/flag-sheet.tsx
📚 Learning: 2025-12-02T22:57:07.791Z
Learnt from: CR
Repo: databuddy-analytics/Databuddy PR: 0
File: .cursor/rules/ultracite.mdc:0-0
Timestamp: 2025-12-02T22:57:07.791Z
Learning: Applies to **/*.{ts,tsx,jsx} : Don't use unnecessary fragments.
Applied to files:
apps/dashboard/app/(main)/websites/[id]/flags/_components/flag-sheet.tsx
📚 Learning: 2025-12-02T22:57:07.791Z
Learnt from: CR
Repo: databuddy-analytics/Databuddy PR: 0
File: .cursor/rules/ultracite.mdc:0-0
Timestamp: 2025-12-02T22:57:07.791Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Use standard constants instead of approximated literals.
Applied to files:
apps/dashboard/app/(main)/websites/[id]/flags/_components/flag-examples.tsx
🧬 Code graph analysis (4)
apps/api/src/index.ts (1)
apps/api/src/routes/webhooks/flag-scheduler.ts (1)
flagSchedulerWebhook(12-83)
packages/db/src/drizzle/schema.ts (1)
packages/shared/src/flags/index.ts (1)
RolloutStep(95-95)
packages/rpc/src/services/flag-scheduler.ts (1)
packages/db/src/drizzle/schema.ts (1)
RolloutStep(673-675)
apps/dashboard/app/(main)/websites/[id]/flags/_components/flag-examples.tsx (1)
apps/dashboard/lib/flags/get-examples-strategy.ts (2)
getExamplesDisplayStrategy(21-66)getShouldShowExamples(68-79)
🔇 Additional comments (16)
apps/dashboard/app/(main)/websites/[id]/flags/_components/flag-examples.tsx (1)
329-397: LGTM: Template card rendering is well-structured.The card grid with icon rendering, variant display, and expandable code examples is cleanly implemented. Good use of Phosphor icons with
weight="duotone"per guidelines.apps/dashboard/app/(main)/websites/[id]/flags/_components/flag-sheet.tsx (5)
120-170: LGTM: Form reset logic handles edit/create modes correctly.The
useEffectproperly resets form state based onisOpen,flag, andscheduledata. The dependency array includes all relevant values.
289-312: LGTM: Sheet header with clear edit/create distinction.The header correctly shows different titles and descriptions based on
isEditingstate, with appropriate iconography.
447-520: LGTM: Dependency-aware status validation.Good implementation that prevents activating a flag when its dependencies are inactive, with clear tooltip feedback showing which dependencies are blocking.
665-691: LGTM: Add Step button correctly initializes schedule fields.The button properly sets future time default (1 hour from now), initializes schedule type, and ensures required fields are populated.
793-805: LGTM: Delete step uses immutable filter.The delete handler correctly uses
.filter()to create a new array rather than mutating in place.packages/db/src/drizzle/schema.ts (3)
656-661: LGTM on new enums.The
dbPermissionLevelenum and theflagTypeextension with"multivariant"are well-defined and follow existing conventions.
754-779:flagSchedulestable structure is sound.The table correctly references
flags.idwith cascade delete, and includes appropriate indexes. TheqstashScheduleIdsarray supports the multi-step rollout pattern.
702-704: Column definitions are properly typed; no migration files needed with Drizzle's push strategy.The
variants,dependencies, andenvironmentcolumns are appropriately defined with sensible defaults. Since this project usesdrizzle-kit push(schema-driven approach), runningdb:pushwill synchronize these columns to the database—no explicit migration files are required.apps/api/src/index.ts (1)
27-27: Webhook integration looks correct.The
flagSchedulerWebhookis properly imported and registered in the Elysia middleware pipeline. This integrates the QStash-based scheduling approach where the external service triggers the webhook.Also applies to: 60-60
apps/api/src/routes/webhooks/flag-scheduler.ts (2)
6-10: Schema definition is appropriate.The
webhookBodySchemacorrectly defines the expected payload structure with optional step fields for rollout handling.
79-83: Health endpoint is appropriate.The
/healthendpoint provides basic service status for monitoring.apps/api/src/services/flag-scheduler.ts (3)
8-17: Well-definedExecutableScheduleinterface.The interface properly types the schedule object with optional step-related properties, addressing previous concerns about
anytypes.
30-34: Properly typedupdatesobject.The explicit type annotation for
updatesensures type safety when building flag modifications, addressing previous feedback.
81-86: Good error handling for missing flag updates.The code correctly warns when the flag update fails and avoids cascading/cache invalidation in that case.
packages/rpc/src/routers/flag-schedules.ts (1)
206-255: Delete handler implementation is correct.The delete handler correctly:
- Looks up the schedule first by
input.id- Fetches the associated flag via
existingSchedule.flagId- Handles QStash deletion failures gracefully without blocking database deletion
- Provides structured logging
The only issue is the authorization gap covered in the earlier comment (lines 35-61).
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/rpc/src/services/flag-scheduler.ts (1)
5-11: Module-level throw blocks application startup in environments without QStash.This will crash the application if
UPSTASH_QSTASH_TOKENis not set, even in test/development environments where scheduling may not be needed.The previous review suggested lazy initialization. Consider implementing that pattern to avoid blocking startup.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
apps/api/src/routes/webhooks/flag-scheduler.ts(1 hunks)apps/dashboard/package.json(1 hunks)packages/rpc/src/services/flag-scheduler.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit inference engine (.cursor/rules/guidelines.mdc)
**/*.{ts,tsx,js,jsx,vue}: Never block paste in<input>or<textarea>elements
Enter submits focused text input; in<textarea>, ⌘/Ctrl+Enter submits; Enter adds newline
Compatible with password managers and 2FA; allow pasting one-time codes
Files:
apps/api/src/routes/webhooks/flag-scheduler.tspackages/rpc/src/services/flag-scheduler.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/guidelines.mdc)
**/*.{ts,tsx,js,jsx}: Trim values to handle text expansion and trailing spaces in form submissions
URL reflects state (deep-link filters/tabs/pagination/expanded panels); prefer libraries likenuqs
Back/Forward buttons restore scroll position
Delay first tooltip in a group; subsequent peers have no delay
Use locale-aware formatting for dates, times, numbers, and currency
Batch layout reads/writes; avoid unnecessary reflows/repaints
Virtualize large lists using libraries likevirtua
**/*.{ts,tsx,js,jsx}: Don't useaccessKeyattribute on any HTML element.
Don't setaria-hidden="true"on focusable elements.
Don't add ARIA roles, states, and properties to elements that don't support them.
Don't use distracting elements like<marquee>or<blink>.
Only use thescopeprop on<th>elements.
Don't assign non-interactive ARIA roles to interactive HTML elements.
Make sure label elements have text content and are associated with an input.
Don't assign interactive ARIA roles to non-interactive HTML elements.
Don't assigntabIndexto non-interactive HTML elements.
Don't use positive integers fortabIndexproperty.
Don't include "image", "picture", or "photo" in img alt prop.
Don't use explicit role property that's the same as the implicit/default role.
Make static elements with click handlers use a valid role attribute.
Always include atitleelement for SVG elements.
Give all elements requiring alt text meaningful information for screen readers.
Make sure anchors have content that's accessible to screen readers.
AssigntabIndexto non-interactive HTML elements witharia-activedescendant.
Include all required ARIA attributes for elements with ARIA roles.
Make sure ARIA properties are valid for the element's supported roles.
Always include atypeattribute for button elements.
Make elements with interactive roles and handlers focusable.
Give heading elements content that's accessible to screen readers (not hidden witharia-hidden).
Always include...
Files:
apps/api/src/routes/webhooks/flag-scheduler.tspackages/rpc/src/services/flag-scheduler.ts
**/*.{ts,tsx,js,jsx,css,scss,sass,less}
📄 CodeRabbit inference engine (.cursor/rules/guidelines.mdc)
**/*.{ts,tsx,js,jsx,css,scss,sass,less}: During drag operations, disable text selection and setinerton dragged element and containers
Animations must be interruptible and input-driven; avoid autoplay
Files:
apps/api/src/routes/webhooks/flag-scheduler.tspackages/rpc/src/services/flag-scheduler.ts
**/*.{ts,tsx,jsx}
📄 CodeRabbit inference engine (.cursor/rules/ultracite.mdc)
**/*.{ts,tsx,jsx}: Use semantic elements instead of role attributes in JSX.
Don't use unnecessary fragments.
Don't pass children as props.
Don't use the return value of React.render.
Make sure all dependencies are correctly specified in React hooks.
Make sure all React hooks are called from the top level of component functions.
Don't forget key props in iterators and collection literals.
Don't destructure props inside JSX components in Solid projects.
Don't define React components inside other components.
Don't use event handlers on non-interactive elements.
Don't assign to React component props.
Don't use bothchildrenanddangerouslySetInnerHTMLprops on the same element.
Don't use dangerous JSX props.
Don't use Array index in keys.
Don't insert comments as text nodes.
Don't assign JSX properties multiple times.
Don't add extra closing tags for components without children.
Use<>...</>instead of<Fragment>...</Fragment>.
Watch out for possible "wrong" semicolons inside JSX elements.
Make sure void (self-closing) elements don't have children.
Don't usetarget="_blank"withoutrel="noopener".
Don't use<img>elements in Next.js projects.
Don't use<head>elements in Next.js projects.
Files:
apps/api/src/routes/webhooks/flag-scheduler.tspackages/rpc/src/services/flag-scheduler.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/ultracite.mdc)
**/*.{ts,tsx}: Don't use primitive type aliases or misleading types.
Don't use empty type parameters in type aliases and interfaces.
Don't use any or unknown as type constraints.
Don't return a value from a function with the return type 'void'.
Don't use the TypeScript directive @ts-ignore.
Don't use TypeScript enums.
Don't add type annotations to variables, parameters, and class properties that are initialized with literal expressions.
Don't use TypeScript namespaces.
Don't use non-null assertions with the!postfix operator.
Don't use parameter properties in class constructors.
Don't use user-defined types.
Useas constinstead of literal types and type annotations.
Use eitherT[]orArray<T>consistently.
Initialize each enum member value explicitly.
Useexport typefor types.
Useimport typefor types.
Make sure all enum members are literal values.
Don't use TypeScript const enum.
Don't declare empty interfaces.
Don't let variables evolve into any type through reassignments.
Don't use the any type.
Don't misuse the non-null assertion operator (!) in TypeScript files.
Don't use implicit any type on variable declarations.
Don't merge interfaces and classes unsafely.
Don't use overload signatures that aren't next to each other.
Use the namespace keyword instead of the module keyword to declare TypeScript namespaces.
Use consistent accessibility modifiers on class properties and methods.
Use function types instead of object types with call signatures.
Don't use void type outside of generic or return types.Do not use types 'any', 'unknown' or 'never', use proper explicit types
Files:
apps/api/src/routes/webhooks/flag-scheduler.tspackages/rpc/src/services/flag-scheduler.ts
!(**/pages/_document.{ts,tsx,jsx})**/*.{ts,tsx,jsx}
📄 CodeRabbit inference engine (.cursor/rules/ultracite.mdc)
Don't import next/document outside of pages/_document.jsx in Next.js projects.
Files:
apps/api/src/routes/webhooks/flag-scheduler.tspackages/rpc/src/services/flag-scheduler.tsapps/dashboard/package.json
**/*.{ts,tsx,html,css}
📄 CodeRabbit inference engine (.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc)
Use
bun build <file.html|file.ts|file.css>instead ofwebpackoresbuildfor bundling
Files:
apps/api/src/routes/webhooks/flag-scheduler.tspackages/rpc/src/services/flag-scheduler.ts
🧠 Learnings (5)
📚 Learning: 2025-12-02T22:57:07.791Z
Learnt from: CR
Repo: databuddy-analytics/Databuddy PR: 0
File: .cursor/rules/ultracite.mdc:0-0
Timestamp: 2025-12-02T22:57:07.791Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Make sure Promise-like statements are handled appropriately.
Applied to files:
apps/api/src/routes/webhooks/flag-scheduler.ts
📚 Learning: 2025-12-02T22:57:07.791Z
Learnt from: CR
Repo: databuddy-analytics/Databuddy PR: 0
File: .cursor/rules/ultracite.mdc:0-0
Timestamp: 2025-12-02T22:57:07.791Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Don't use async functions as Promise executors.
Applied to files:
apps/api/src/routes/webhooks/flag-scheduler.ts
📚 Learning: 2025-12-02T22:57:07.791Z
Learnt from: CR
Repo: databuddy-analytics/Databuddy PR: 0
File: .cursor/rules/ultracite.mdc:0-0
Timestamp: 2025-12-02T22:57:07.791Z
Learning: Applies to **/*.{ts,tsx} : Don't misuse the non-null assertion operator (!) in TypeScript files.
Applied to files:
apps/api/src/routes/webhooks/flag-scheduler.ts
📚 Learning: 2025-12-02T22:57:07.791Z
Learnt from: CR
Repo: databuddy-analytics/Databuddy PR: 0
File: .cursor/rules/ultracite.mdc:0-0
Timestamp: 2025-12-02T22:57:07.791Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Don't use explicit role property that's the same as the implicit/default role.
Applied to files:
apps/api/src/routes/webhooks/flag-scheduler.ts
📚 Learning: 2025-12-02T22:57:07.791Z
Learnt from: CR
Repo: databuddy-analytics/Databuddy PR: 0
File: .cursor/rules/ultracite.mdc:0-0
Timestamp: 2025-12-02T22:57:07.791Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Don't hardcode sensitive data like API keys and tokens.
Applied to files:
packages/rpc/src/services/flag-scheduler.ts
🧬 Code graph analysis (2)
apps/api/src/routes/webhooks/flag-scheduler.ts (2)
packages/db/src/drizzle/schema.ts (1)
flagSchedules(754-779)apps/api/src/services/flag-scheduler.ts (1)
executeSchedule(19-95)
packages/rpc/src/services/flag-scheduler.ts (1)
packages/db/src/drizzle/schema.ts (1)
RolloutStep(673-675)
🔇 Additional comments (4)
apps/api/src/routes/webhooks/flag-scheduler.ts (2)
18-46: QStash signature verification properly implemented.The signature verification addresses the security concern from the previous review. The implementation correctly validates the
Upstash-Signatureheader before processing.
83-101: Schedule execution logic looks correct.The rollout step handling properly checks for already-executed steps and passes the appropriate parameters to
executeSchedule. Theanytype concerns from the previous review appear to have been addressed.packages/rpc/src/services/flag-scheduler.ts (2)
164-178: Delete function properly simplified.The nested try-catch blocks from the previous review have been consolidated. The current approach of logging and rethrowing allows callers to decide how to handle deletion failures.
17-60: Schedule creation implementation looks good.The function correctly converts the scheduled time to a Unix timestamp, publishes to QStash with proper headers, validates the response, and includes appropriate logging and error handling.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
apps/api/src/routes/webhooks/flag-scheduler.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit inference engine (.cursor/rules/guidelines.mdc)
**/*.{ts,tsx,js,jsx,vue}: Never block paste in<input>or<textarea>elements
Enter submits focused text input; in<textarea>, ⌘/Ctrl+Enter submits; Enter adds newline
Compatible with password managers and 2FA; allow pasting one-time codes
Files:
apps/api/src/routes/webhooks/flag-scheduler.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/guidelines.mdc)
**/*.{ts,tsx,js,jsx}: Trim values to handle text expansion and trailing spaces in form submissions
URL reflects state (deep-link filters/tabs/pagination/expanded panels); prefer libraries likenuqs
Back/Forward buttons restore scroll position
Delay first tooltip in a group; subsequent peers have no delay
Use locale-aware formatting for dates, times, numbers, and currency
Batch layout reads/writes; avoid unnecessary reflows/repaints
Virtualize large lists using libraries likevirtua
**/*.{ts,tsx,js,jsx}: Don't useaccessKeyattribute on any HTML element.
Don't setaria-hidden="true"on focusable elements.
Don't add ARIA roles, states, and properties to elements that don't support them.
Don't use distracting elements like<marquee>or<blink>.
Only use thescopeprop on<th>elements.
Don't assign non-interactive ARIA roles to interactive HTML elements.
Make sure label elements have text content and are associated with an input.
Don't assign interactive ARIA roles to non-interactive HTML elements.
Don't assigntabIndexto non-interactive HTML elements.
Don't use positive integers fortabIndexproperty.
Don't include "image", "picture", or "photo" in img alt prop.
Don't use explicit role property that's the same as the implicit/default role.
Make static elements with click handlers use a valid role attribute.
Always include atitleelement for SVG elements.
Give all elements requiring alt text meaningful information for screen readers.
Make sure anchors have content that's accessible to screen readers.
AssigntabIndexto non-interactive HTML elements witharia-activedescendant.
Include all required ARIA attributes for elements with ARIA roles.
Make sure ARIA properties are valid for the element's supported roles.
Always include atypeattribute for button elements.
Make elements with interactive roles and handlers focusable.
Give heading elements content that's accessible to screen readers (not hidden witharia-hidden).
Always include...
Files:
apps/api/src/routes/webhooks/flag-scheduler.ts
**/*.{ts,tsx,js,jsx,css,scss,sass,less}
📄 CodeRabbit inference engine (.cursor/rules/guidelines.mdc)
**/*.{ts,tsx,js,jsx,css,scss,sass,less}: During drag operations, disable text selection and setinerton dragged element and containers
Animations must be interruptible and input-driven; avoid autoplay
Files:
apps/api/src/routes/webhooks/flag-scheduler.ts
**/*.{ts,tsx,jsx}
📄 CodeRabbit inference engine (.cursor/rules/ultracite.mdc)
**/*.{ts,tsx,jsx}: Use semantic elements instead of role attributes in JSX.
Don't use unnecessary fragments.
Don't pass children as props.
Don't use the return value of React.render.
Make sure all dependencies are correctly specified in React hooks.
Make sure all React hooks are called from the top level of component functions.
Don't forget key props in iterators and collection literals.
Don't destructure props inside JSX components in Solid projects.
Don't define React components inside other components.
Don't use event handlers on non-interactive elements.
Don't assign to React component props.
Don't use bothchildrenanddangerouslySetInnerHTMLprops on the same element.
Don't use dangerous JSX props.
Don't use Array index in keys.
Don't insert comments as text nodes.
Don't assign JSX properties multiple times.
Don't add extra closing tags for components without children.
Use<>...</>instead of<Fragment>...</Fragment>.
Watch out for possible "wrong" semicolons inside JSX elements.
Make sure void (self-closing) elements don't have children.
Don't usetarget="_blank"withoutrel="noopener".
Don't use<img>elements in Next.js projects.
Don't use<head>elements in Next.js projects.
Files:
apps/api/src/routes/webhooks/flag-scheduler.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/ultracite.mdc)
**/*.{ts,tsx}: Don't use primitive type aliases or misleading types.
Don't use empty type parameters in type aliases and interfaces.
Don't use any or unknown as type constraints.
Don't return a value from a function with the return type 'void'.
Don't use the TypeScript directive @ts-ignore.
Don't use TypeScript enums.
Don't add type annotations to variables, parameters, and class properties that are initialized with literal expressions.
Don't use TypeScript namespaces.
Don't use non-null assertions with the!postfix operator.
Don't use parameter properties in class constructors.
Don't use user-defined types.
Useas constinstead of literal types and type annotations.
Use eitherT[]orArray<T>consistently.
Initialize each enum member value explicitly.
Useexport typefor types.
Useimport typefor types.
Make sure all enum members are literal values.
Don't use TypeScript const enum.
Don't declare empty interfaces.
Don't let variables evolve into any type through reassignments.
Don't use the any type.
Don't misuse the non-null assertion operator (!) in TypeScript files.
Don't use implicit any type on variable declarations.
Don't merge interfaces and classes unsafely.
Don't use overload signatures that aren't next to each other.
Use the namespace keyword instead of the module keyword to declare TypeScript namespaces.
Use consistent accessibility modifiers on class properties and methods.
Use function types instead of object types with call signatures.
Don't use void type outside of generic or return types.Do not use types 'any', 'unknown' or 'never', use proper explicit types
Files:
apps/api/src/routes/webhooks/flag-scheduler.ts
!(**/pages/_document.{ts,tsx,jsx})**/*.{ts,tsx,jsx}
📄 CodeRabbit inference engine (.cursor/rules/ultracite.mdc)
Don't import next/document outside of pages/_document.jsx in Next.js projects.
Files:
apps/api/src/routes/webhooks/flag-scheduler.ts
**/*.{ts,tsx,html,css}
📄 CodeRabbit inference engine (.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc)
Use
bun build <file.html|file.ts|file.css>instead ofwebpackoresbuildfor bundling
Files:
apps/api/src/routes/webhooks/flag-scheduler.ts
🧠 Learnings (6)
📚 Learning: 2025-12-02T22:57:07.791Z
Learnt from: CR
Repo: databuddy-analytics/Databuddy PR: 0
File: .cursor/rules/ultracite.mdc:0-0
Timestamp: 2025-12-02T22:57:07.791Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Make sure Promise-like statements are handled appropriately.
Applied to files:
apps/api/src/routes/webhooks/flag-scheduler.ts
📚 Learning: 2025-12-02T22:57:07.791Z
Learnt from: CR
Repo: databuddy-analytics/Databuddy PR: 0
File: .cursor/rules/ultracite.mdc:0-0
Timestamp: 2025-12-02T22:57:07.791Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Don't use async functions as Promise executors.
Applied to files:
apps/api/src/routes/webhooks/flag-scheduler.ts
📚 Learning: 2025-12-02T22:57:07.791Z
Learnt from: CR
Repo: databuddy-analytics/Databuddy PR: 0
File: .cursor/rules/ultracite.mdc:0-0
Timestamp: 2025-12-02T22:57:07.791Z
Learning: Applies to **/*.{ts,tsx} : Don't misuse the non-null assertion operator (!) in TypeScript files.
Applied to files:
apps/api/src/routes/webhooks/flag-scheduler.ts
📚 Learning: 2025-12-02T22:57:07.791Z
Learnt from: CR
Repo: databuddy-analytics/Databuddy PR: 0
File: .cursor/rules/ultracite.mdc:0-0
Timestamp: 2025-12-02T22:57:07.791Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Don't use explicit role property that's the same as the implicit/default role.
Applied to files:
apps/api/src/routes/webhooks/flag-scheduler.ts
📚 Learning: 2025-12-02T22:57:07.791Z
Learnt from: CR
Repo: databuddy-analytics/Databuddy PR: 0
File: .cursor/rules/ultracite.mdc:0-0
Timestamp: 2025-12-02T22:57:07.791Z
Learning: Applies to **/*.{ts,tsx} : Don't use non-null assertions with the `!` postfix operator.
Applied to files:
apps/api/src/routes/webhooks/flag-scheduler.ts
📚 Learning: 2025-12-02T22:57:07.791Z
Learnt from: CR
Repo: databuddy-analytics/Databuddy PR: 0
File: .cursor/rules/ultracite.mdc:0-0
Timestamp: 2025-12-02T22:57:07.791Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Don't hardcode sensitive data like API keys and tokens.
Applied to files:
apps/api/src/routes/webhooks/flag-scheduler.ts
🧬 Code graph analysis (1)
apps/api/src/routes/webhooks/flag-scheduler.ts (2)
packages/db/src/drizzle/schema.ts (1)
flagSchedules(754-779)apps/api/src/services/flag-scheduler.ts (1)
executeSchedule(19-95)
🔇 Additional comments (1)
apps/api/src/routes/webhooks/flag-scheduler.ts (1)
13-61: Overall webhook design, validation, and logging look solidEnd‑to‑end flow is well structured: QStash signature verification, early config/env checks for signing keys (avoiding non‑null
!, which matches past learnings), Zod request‑body validation, clear skip reasons (disabled,already_executed,step_already_executed), and a simple health endpoint. The logging fields around verification and execution will be useful operationally.Once the env and
scheduleIdalignment concerns are addressed, this endpoint looks production‑ready.Also applies to: 77-133
Done! Make sure to have these envs: UPSTASH_QSTASH_URL= and if you are testing localy using ngrok you wil need to hardcode FLAG_SCHEDULER_DESTINATION variable,
|
|
Hey @izadoesdev , any issue you closed the pr without merge ? |
nope, i was fixing some issues with the git history and it auto closed, you can re open |
I cant reopen due to permissions, you can . |
7 participants
Fixes #190
Description
Here is the quick practical demonstration for changes this pr introduces:
demo-1764750770522.1.1.mp4
Checklist
Summary by CodeRabbit
New Features
Improvements
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.
Note
Introduce multivariant flags with environment-aware evaluation, a background flag scheduler with APIs, a server-side SDK manager, and UI tools (variants, dependencies, schedules, examples).
variant, non-boolean values), environment-scoped queries/telemetry, and variant selection logic; extend tests accordingly.startFlagScheduler/stopFlagScheduler) and processing for one-off enable/disable and rollout-step updates.flagSchedulesrouter (create/update/get/delete); extendflagsrouter with variants, dependencies, circular checks, and cascading status updates.flagstable withvariants,dependencies,environment.flag_schedulestable and related enums/indexes.createServerFlagsManager) with per-request evaluation (supportsenvironment); adjust core/typing to allow multivariant values.Written by Cursor Bugbot for commit 8665e01. This will update automatically on new commits. Configure here.