Feature flags v1: schema, evaluator, routes, dashboard#1392
Draft
mantrakp04 wants to merge 8 commits intodevfrom
Draft
Feature flags v1: schema, evaluator, routes, dashboard#1392mantrakp04 wants to merge 8 commits intodevfrom
mantrakp04 wants to merge 8 commits intodevfrom
Conversation
Definitions live in branch config (free branch/env overrides). Pure
deterministic evaluator shared by backend and (future) SDKs, MurmurHash3
bucketing, kill switch, dependsOn, holdouts, weighted multivariate.
Adds /feature-flags/{bootstrap,evaluate} routes, dashboard CRUD, seed
data, and an example demo page.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a feature-flag system: runtime-safe types, hashing, deterministic evaluator, new backend bootstrap/evaluate endpoints, dashboard UI for listing/creating/editing flags, client SDK/hooks and client-app integrations, seed/demo data, and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client/SDK/Dashboard
participant API as Backend API
participant Config as Resolved Config Store
participant Evaluator as Evaluator
participant Hash as Bucketing
Client->>API: POST /feature-flags/evaluate { flag_keys?, distinct_id?, context? }
API->>Config: resolve tenancy.config.featureFlags
API->>Evaluator: evaluateFlag(flagId, config, context)
Evaluator->>Evaluator: check enabled / killSwitch / dependsOn
Evaluator->>Hash: compute holdout bucket (if configured)
Hash-->>Evaluator: holdout decision
Evaluator->>Evaluator: evaluate rules (conditions, priority)
Evaluator->>Hash: rollout & weightedVariant buckets (stickyBy/distinctId)
Hash-->>Evaluator: selected variant
Evaluator-->>API: result { flag_key, variant_key, value, reason, rule_id? }
API-->>Client: 200 JSON results (or 304 for bootstrap if ETag matches)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
Implements a first-cut, config-backed feature flag system with a shared deterministic evaluator, backend API endpoints for evaluation/bootstrap, dashboard CRUD UI, seed demo flags, and accompanying tests/demo documentation updates.
Changes:
- Added shared feature-flag config types, hashing, and evaluator logic to
@stackframe/stack-shared. - Introduced backend routes for
/api/latest/feature-flags/evaluateand/bootstrap, plus E2E coverage. - Added a new Dashboard “Feature Flags” app with list/detail editing, and seeded demo flags + a demo page.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/stack-shared/src/interface/crud/feature-flags.ts | Re-exports public feature-flag types/operators for consumers. |
| packages/stack-shared/src/feature-flags/types.ts | Adds runtime TS types for feature flag config and evaluation results. |
| packages/stack-shared/src/feature-flags/hashing.ts | Adds deterministic Murmur3-based bucketing and weighted variant selection. |
| packages/stack-shared/src/feature-flags/evaluator.ts | Adds pure evaluator + helpers (rule matching, dependencies, holdouts, key→id lookup). |
| packages/stack-shared/src/config/schema.ts | Extends branch config schema/defaults to include feature flag definitions and validation. |
| packages/stack-shared/src/config/schema-fuzzer.test.ts | Extends schema fuzzing inputs to cover the new featureFlags config surface. |
| packages/stack-shared/src/apps/apps-config.ts | Registers new “feature-flags” app metadata in shared app registry. |
| examples/demo/src/app/feature-flags-demo/page.tsx | Adds demo page that calls the evaluate endpoint with a publishable key. |
| docs/src/components/mdx/app-card.tsx | Adds icon mapping for the new “feature-flags” app in docs UI. |
| apps/e2e/tests/backend/endpoints/api/v1/feature-flags.test.ts | Adds E2E tests for evaluate + bootstrap and local-vs-server evaluator parity. |
| apps/dashboard/src/lib/apps-frontend.tsx | Adds Dashboard navigation entry for “Feature Flags”. |
| apps/dashboard/src/app/(main)/(protected)/projects/[projectId]/feature-flags/page.tsx | Adds route entrypoint for feature flags list page. |
| apps/dashboard/src/app/(main)/(protected)/projects/[projectId]/feature-flags/page-client.tsx | Implements flag list + create dialog wiring via config updates. |
| apps/dashboard/src/app/(main)/(protected)/projects/[projectId]/feature-flags/[flagId]/page.tsx | Adds route entrypoint for feature flag detail page. |
| apps/dashboard/src/app/(main)/(protected)/projects/[projectId]/feature-flags/[flagId]/page-client.tsx | Implements detail editor (metadata/variants/rules JSON) + local debug evaluator. |
| apps/backend/src/app/api/latest/feature-flags/evaluate/route.ts | Adds server-side evaluation endpoint with key→id resolution and result shaping. |
| apps/backend/src/app/api/latest/feature-flags/bootstrap/route.ts | Adds bootstrap endpoint returning definitions + key→id map + content version. |
| apps/backend/prisma/seed.ts | Seeds demo feature flags into the internal project for first-run experience. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Greptile SummaryThis PR introduces a complete feature flag system: a config-schema-backed definition model, a pure deterministic evaluator shared by the backend and SDK, bootstrap/evaluate routes with ETag caching, a dashboard CRUD UI, and client SDK helpers with React hooks. The architecture is well-thought-out — client-key context spoofing is blocked, regex patterns are validated and cached with a ReDoS guard, the MurmurHash3 bucketing is stable, and cross-reference validation (dependsOn, holdoutId, variantKey) is enforced at save time.
Confidence Score: 3/5Safe to merge with caution — the not_in/in operator footgun can silently produce wrong flag evaluations for misconfigured rules. One P1 finding (in/not_in operator value type not validated in schema) can cause silent wrong targeting for any rule that uses those operators with a non-array value. The PATCH non-atomic read-modify-write is a pre-existing pattern concern. Core evaluator, hashing, auth controls, and dashboard flow are solid. packages/stack-shared/src/config/schema.ts — condition value type validation for in/not_in operators; apps/backend/src/app/api/latest/internal/config/override/[level]/route.tsx — PATCH atomicity. Important Files Changed
Sequence DiagramsequenceDiagram
participant SDK as Client SDK
participant Boot as GET /bootstrap
participant Eval as POST /evaluate
participant Ev as evaluator.ts
participant Cfg as Branch Config
SDK->>Boot: GET bootstrap (If-None-Match: etag)
Boot->>Cfg: read featureFlags config
Boot->>Boot: murmur3 hash → version + etag
alt ETag matches
Boot-->>SDK: 304 Not Modified
else Config changed
Boot-->>SDK: 200 flags + holdouts + version
end
SDK->>Eval: POST evaluate with flag_keys
Eval->>Cfg: read featureFlags config
alt client auth type
Eval->>Eval: strip user/team/context/cohorts
Eval->>Eval: use verified server-side attrs only
else server or admin auth
Eval->>Eval: accept caller-supplied targeting context
end
Eval->>Ev: evaluateFlag per requested key
Ev->>Ev: kill_switch, disabled, dep, holdout, rules, default
Eval-->>SDK: results map with variantKey, value, reason
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
packages/stack-shared/src/config/schema.ts:318-322
**`not_in` / `in` condition values are not type-validated**
The condition `value` is declared as `yupMixed()`, so the schema accepts any JSON value for every operator. For the `not_in` operator specifically, `applyOperator` is:
```ts
case "not_in": { return !(Array.isArray(expected) && expected.includes(actual)); }
```
When `expected` is not an array (e.g. someone types `"admin"` as a plain string instead of `["admin"]`), `Array.isArray(expected)` is `false` so the whole expression becomes `!false === true`, meaning **every user matches the rule unconditionally**. This silently turns an exclusion rule into an "always serve this variant" rule with no error at save time.
The same inverted failure applies to `in`: a non-array value makes it always-false, silently blocking everyone.
Consider adding a `yup.when("operator", …)` refinement that enforces `Array.isArray(value)` when `operator` is `in` or `not_in`.
### Issue 2 of 3
apps/backend/src/app/api/latest/internal/config/override/[level]/route.tsx:321-337
**Non-atomic read-modify-write in PATCH handler**
The PATCH route now performs three separate database operations: `get` → (local `override`) → `set`. If two dashboard users push config patches concurrently, both may read the same `oldConfig`, compute independent `newConfig` values, and the second `set` will silently discard whichever changes the first `set` already committed.
The previous implementation delegated to `levelConfig.override()`, which presumably handled the merge atomically (or at a minimum in a single operation). Replacing it with an explicit get/set pair opens a TOCTOU window that didn't exist before. For feature-flag config writes from the dashboard this is unlikely to cause production incidents, but worth noting if atomic writes are achievable here.
### Issue 3 of 3
packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts:2692-2695
**`useFeatureFlags` cache key is order-sensitive**
`_serializeFeatureFlagKeys` uses `JSON.stringify(keys)` to derive the cache key. `["a","b"]` and `["b","a"]` stringify differently and produce two independent cache entries, even though the requested flags are identical. A component calling `useFeatureFlags(["checkout","pricing"])` and another calling `useFeatureFlags(["pricing","checkout"])` will each trigger separate network requests and maintain separate subscriptions.
Consider sorting keys before serializing (e.g. `JSON.stringify([...keys].sort())`) so the cache is order-insensitive and components naturally share entries.
Reviews (4): Last reviewed commit: "Enhance feature flag evaluation and conf..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (2)
packages/stack-shared/src/feature-flags/evaluator.ts (1)
18-25: Avoid the cast in the dotted-path lookup helper.
(cursor as Record<string, unknown>)sidesteps the type system in a core helper. You can keep this typed withReflect.getafter the object/null guard instead of casting.As per coding guidelines "Do NOT use
as/any/type casts or anything else like that to bypass the type system. Most of the time a place where you would use type casts is not one where you actually need them".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stack-shared/src/feature-flags/evaluator.ts` around lines 18 - 25, The helper getDottedAttribute currently bypasses the type system by casting cursor to Record<string, unknown>; instead, after the null/object guard inside the loop use Reflect.get(cursor, part) to access the property without an explicit cast, preserving proper typing for EvalContext and cursor and returning undefined when the guard fails; update references to cursor, parts, and attribute accordingly and remove the (cursor as Record<string, unknown>) cast.apps/e2e/tests/backend/endpoints/api/v1/feature-flags.test.ts (1)
6-8: Use path notation for the config update helper.Writing the whole
featureFlagsobject through the plain shape here can overwrite sibling config fields and makes these tests less representative of production updates.Suggested change
async function setupProjectWithFlags(featureFlags: FeatureFlagsConfig) { await Project.createAndSwitch(); - await Project.updatePushedConfig({ featureFlags }); + await Project.updatePushedConfig({ "featureFlags": featureFlags }); }As per coding guidelines "When making config updates, use path notation (
{ "path.to.field": my-value }) to avoid overwriting sibling properties".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/tests/backend/endpoints/api/v1/feature-flags.test.ts` around lines 6 - 8, The test currently calls Project.updatePushedConfig with a full nested object (updatePushedConfig({ featureFlags })) which can overwrite sibling config fields; change the call in setupProjectWithFlags to use path notation instead (e.g., Project.updatePushedConfig({ "featureFlags": featureFlags })) so only the featureFlags path is updated and siblings are preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/backend/prisma/seed.ts`:
- Around line 207-210: The "is-employee" rule currently uses operator "contains"
on attribute "user.email", which can match unintended substrings; change it to
an anchored domain match by using a proper operator like "endsWith" with value
"@stack-auth.com" or switch to a regex operator (e.g., "matches") with a pattern
that anchors the domain (such as "@stack-auth\\.com$") so only emails ending in
the company domain are considered internal.
In `@apps/backend/src/app/api/latest/feature-flags/bootstrap/route.ts`:
- Around line 35-48: The version hash is computed from JSON.stringify({ flags:
flagsById, flagIdsByKey, holdouts }) which is sensitive to insertion order;
implement a canonicalization step that deep-sorts object keys (and arrays
deterministically) before stringifying so reordering-only changes don't change
the payload; create or use a helper like deepSortObject and call it on { flags:
flagsById, flagIdsByKey, holdouts } prior to passing into
hashingInternal.murmur3_32 to produce the stable version value used by version.
In `@apps/backend/src/app/api/latest/feature-flags/evaluate/route.ts`:
- Around line 36-38: The request schema currently defines cohorts as
yupRecord(yupString(), yupMixed()) and the handler coerces cohort values with
Boolean(...), which incorrectly turns values like "false" into true; change the
schema to validate cohorts as Record<string, boolean> by replacing yupMixed()
with yupBoolean() (use yupRecord(yupString(), yupBoolean()) for the cohorts
field) and remove any Boolean(...) coercion in the handler so the cohorts object
(symbol: cohorts) is passed through unchanged; make the same change for the
other occurrence referenced (lines 58-60) where cohort values are coerced.
In
`@apps/dashboard/src/app/`(main)/(protected)/projects/[projectId]/feature-flags/[flagId]/page-client.tsx:
- Around line 225-282: The save/delete handlers (handleSaveMetadata,
handleSaveVariants, handleSaveRules, handleDelete) currently always show toast
or navigate regardless of whether updateConfig succeeded; useUpdateConfig
returns a boolean result, so await the updateConfig call, check its returned ok
value, and only call toast or router.push when ok is true; if ok is false,
surface an error (e.g., alert or toast with failure) and do not redirect. Ensure
you reference the updateConfig(...) invocation inside each handler and gate
post-success side effects on the returned boolean.
In
`@apps/dashboard/src/app/`(main)/(protected)/projects/[projectId]/feature-flags/page-client.tsx:
- Around line 47-55: The current flag factory returns a live-but-unconfigured
flag object (key, type, enabled: true, killSwitch, defaultVariantKey, variants,
rules) which makes non-boolean flags evaluate to undefined; modify the factory
so that when type !== 'boolean' it either sets enabled: false (start as a
disabled draft) or seeds an initial variant and sets defaultVariantKey (e.g.,
create a default entry in variants and assign defaultVariantKey) instead of
leaving defaultVariantKey undefined; update the returned object construction
that builds key/type/enabled/defaultVariantKey/variants to perform this
conditional initialization.
In `@apps/e2e/tests/backend/endpoints/api/v1/feature-flags.test.ts`:
- Around line 11-285: The test suite is missing coverage for two behaviors:
falling back to the authenticated user's id when distinct_id is omitted and
bootstrap revalidation via If-None-Match; add two it tests in this file that use
setupProjectWithFlags and niceBackendFetch: (1) an "/feature-flags/evaluate"
test that authenticates as a user (simulate accessType that ties to an
authenticated user or include an auth header) calls POST without distinct_id and
asserts the returned result reflects that user's id (same deterministic variant
as when you pass distinct_id), and (2) a "/feature-flags/bootstrap" test that
first GETs the bootstrap to capture body.version and then issues a second GET
with an If-None-Match header set to that version and asserts a 304/Not Modified
response (no body) to verify revalidation behavior; place these as additional
it(...) blocks alongside the existing tests referencing niceBackendFetch,
setupProjectWithFlags, and the endpoints "/api/latest/feature-flags/evaluate"
and "/api/latest/feature-flags/bootstrap".
In `@examples/demo/src/app/feature-flags-demo/page.tsx`:
- Around line 15-27: The code silently falls back to empty strings for
NEXT_PUBLIC_STACK_* env vars in evaluateFlags, which masks misconfiguration;
instead, validate apiUrl, projectId, and publishableClientKey at startup (or at
start of evaluateFlags) and throw a clear error if any are missing so the app
fails fast. Replace uses of apiUrl ?? "" / projectId ?? "" /
publishableClientKey ?? "" inside evaluateFlags with the validated non-null
strings (or call a small helper like ensureEnv('NEXT_PUBLIC_STACK_API_URL',
apiUrl)) and propagate/throw a descriptive error if validation fails so the
fetch call never runs with empty/undefined values. Ensure to reference and
update the constants apiUrl, projectId, publishableClientKey and the
evaluateFlags function when making this change.
In `@packages/stack-shared/src/config/schema.ts`:
- Around line 239-291: Add a schema-level test on the feature-flag object (the
schema that contains variants, defaultVariantKey and rules) that collects the
set of variant ids from the variants record and validates that
defaultVariantKey, every rules.*.variantKey, and every key referenced in
rules.*.variantWeights exist in that set; use yup.TestContext (this.createError)
to return clear errors for missing variant ids and reference the existing
symbols: variants, defaultVariantKey, rules, variantKey, variantWeights,
yupRecord and userSpecifiedIdSchema to locate the parent schema; implement the
test as a function that reads this.parent.variants to build the allowed ids,
iterates rules (this.parent.rules) to check each referenced id, and calls
this.createError with appropriate path (e.g. `${this.path}.defaultVariantKey` or
`${this.path}.rules.${ruleId}.variantWeights`) when a missing id is found.
In `@packages/stack-shared/src/feature-flags/evaluator.ts`:
- Around line 205-210: The dependsOn check currently treats only true, non-empty
strings, and non-zero numbers as "met", rejecting JSON objects/arrays; update
evaluateFlag's truthiness logic (the depTruthy computation inside the dependsOn
branch) to use a shared isTruthy helper that returns true for: boolean true,
non-empty strings, non-zero numbers, non-null objects with at least one own key,
non-empty arrays (length > 0); return false for null/undefined/empty
objects/arrays/zero/empty string/false. Replace the inline depTruthy expression
with a call to isTruthy(dep.value) and reuse that helper wherever flag
truthiness is needed (keeping defaultResult("dep_unmet") behavior unchanged).
- Around line 61-67: The evaluator's "regex" branch currently compiles
tenant-supplied patterns on each evaluation (case "regex") which risks ReDoS and
silently swallows pattern errors; change the flow so regex patterns are
validated and pre-compiled at flag creation/update (not in evaluate), store the
compiled/validated RegExp (or a safe-regex wrapper) on the flag object, and in
evaluate use the cached compiled instance's test method without constructing a
new RegExp or relying on a bare catch; also ensure creation/update path rejects
invalid or unsafe patterns (or use a safe-regex library with timeouts) so
evaluate never compiles untrusted input.
- Around line 28-43: The compareSemver implementation is not semver-compliant
because it treats prerelease and build metadata as ordinary sortable tokens;
update compareSemver to parse and compare semver per the spec: split a version
into core (major.minor.patch numeric), prerelease (dot-separated identifiers)
and ignore build metadata entirely, then compare cores numerically, and if equal
treat a version without prerelease as higher than one with prerelease; when
comparing prerelease identifiers apply semver rules (compare numeric identifiers
numerically, non-numeric lexically, numeric < non-numeric, longer list wins only
if all previous equal). Modify the parsing logic in compareSemver and use or
replace stringCompare with the semver identifier comparison rules so that
compareSemver("1.0.0","1.0.0-alpha") > 0 and builds are ignored.
---
Nitpick comments:
In `@apps/e2e/tests/backend/endpoints/api/v1/feature-flags.test.ts`:
- Around line 6-8: The test currently calls Project.updatePushedConfig with a
full nested object (updatePushedConfig({ featureFlags })) which can overwrite
sibling config fields; change the call in setupProjectWithFlags to use path
notation instead (e.g., Project.updatePushedConfig({ "featureFlags":
featureFlags })) so only the featureFlags path is updated and siblings are
preserved.
In `@packages/stack-shared/src/feature-flags/evaluator.ts`:
- Around line 18-25: The helper getDottedAttribute currently bypasses the type
system by casting cursor to Record<string, unknown>; instead, after the
null/object guard inside the loop use Reflect.get(cursor, part) to access the
property without an explicit cast, preserving proper typing for EvalContext and
cursor and returning undefined when the guard fails; update references to
cursor, parts, and attribute accordingly and remove the (cursor as
Record<string, unknown>) cast.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a6f772a3-4cbc-4e0f-9cbc-074953b46d9c
📒 Files selected for processing (18)
apps/backend/prisma/seed.tsapps/backend/src/app/api/latest/feature-flags/bootstrap/route.tsapps/backend/src/app/api/latest/feature-flags/evaluate/route.tsapps/dashboard/src/app/(main)/(protected)/projects/[projectId]/feature-flags/[flagId]/page-client.tsxapps/dashboard/src/app/(main)/(protected)/projects/[projectId]/feature-flags/[flagId]/page.tsxapps/dashboard/src/app/(main)/(protected)/projects/[projectId]/feature-flags/page-client.tsxapps/dashboard/src/app/(main)/(protected)/projects/[projectId]/feature-flags/page.tsxapps/dashboard/src/lib/apps-frontend.tsxapps/e2e/tests/backend/endpoints/api/v1/feature-flags.test.tsdocs/src/components/mdx/app-card.tsxexamples/demo/src/app/feature-flags-demo/page.tsxpackages/stack-shared/src/apps/apps-config.tspackages/stack-shared/src/config/schema-fuzzer.test.tspackages/stack-shared/src/config/schema.tspackages/stack-shared/src/feature-flags/evaluator.tspackages/stack-shared/src/feature-flags/hashing.tspackages/stack-shared/src/feature-flags/types.tspackages/stack-shared/src/interface/crud/feature-flags.ts
- Updated regex condition handling in feature flags to ensure valid patterns are enforced. - Enhanced the evaluation logic to handle distinct user IDs and server-side context more effectively. - Improved response structure for feature flag evaluations, including support for conditional headers and optimized payloads. - Adjusted the default state of feature flags in the dashboard to be disabled. - Added comprehensive tests for new evaluation scenarios and response behaviors.
- Implemented `evaluateFeatureFlags` method in `StackClientInterface` to handle feature flag evaluations via the API. - Added corresponding tests for feature flag evaluation in `client-interface.test.ts` to ensure correct request handling and response validation. - Introduced new types for feature flag requests and responses in `feature-flags.ts` to standardize data structures. - Enhanced the `StackClientApp` interface with methods for retrieving single and multiple feature flags, including React hooks for integration. - Updated documentation to reflect new feature flag functionalities and usage examples.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
packages/stack-shared/src/feature-flags/evaluator.ts (1)
71-81: Consider implementing a size limit or LRU eviction for the regex cache.The
regexCacheMap grows indefinitely as new unique patterns are encountered. While this trades memory for repeated regex compilation performance, consider adding a maximum size limit (e.g., LRU eviction) to prevent unbounded memory growth in long-running servers with many distinct patterns. The length validation on individual patterns (maxFeatureFlagRegexPatternLength) caps individual entry size, but doesn't limit the total number of cached entries.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stack-shared/src/feature-flags/evaluator.ts` around lines 71 - 81, The regexCache Map in getCompiledRegex grows without bound; implement a bounded LRU-style eviction. Add a constant (e.g., REGEX_CACHE_MAX = 1000) and update getCompiledRegex so that on cache hit you promote the key to mark it recent (delete then set), and on inserting a new compiled RegExp you check regexCache.size and evict the oldest entry (use regexCache.keys().next().value) if size > REGEX_CACHE_MAX; alternatively, replace the Map with an LRU cache library but ensure getCompiledRegex and the regexCache symbol behavior are preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/backend/src/app/api/latest/feature-flags/evaluate/route.ts`:
- Around line 51-69: The evalContext currently allows client-authenticated
requests to supply distinct_id (via the else branch using body.distinct_id),
enabling client-side spoofing; update the else/unauthorized-targeting branch so
distinctId is taken only from auth.user?.id (remove body.distinct_id) and ensure
userId and user fields likewise come from auth (e.g., userId: auth.user?.id and
user built from auth.user), preserving the existing
callerCanSupplyTargetingContext check and leaving the auth.type !== "client"
branch unchanged.
In `@packages/stack-shared/src/feature-flags/types.ts`:
- Around line 25-40: Replace the fragile, hand-rolled ReDoS checks in
getFeatureFlagRegexPatternError with a proven ReDoS-safe engine or validator:
remove or keep only the simple length check (maxFeatureFlagRegexPatternLength)
and instead use a library like re2 or safe-regex to validate the pattern (call
the library from getFeatureFlagRegexPatternError before constructing new RegExp)
so you guarantee linear-time matching; ensure the function returns the
library-provided error message on invalid patterns and still returns undefined
for valid ones.
In `@packages/stack-shared/src/interface/client-interface.ts`:
- Around line 552-568: The evaluateFeatureFlags method currently calls
sendClientRequest without passing the requestType so it always defaults to
client access; update evaluateFeatureFlags to accept a requestType parameter (or
forward an existing one) and pass it as the fourth argument to sendClientRequest
so the X-Stack-Access-Type header reflects the caller (e.g., call
sendClientRequest(..., session, requestType)). Ensure you update the
evaluateFeatureFlags signature and any call sites to supply the appropriate
requestType and preserve existing behavior when omitted.
---
Nitpick comments:
In `@packages/stack-shared/src/feature-flags/evaluator.ts`:
- Around line 71-81: The regexCache Map in getCompiledRegex grows without bound;
implement a bounded LRU-style eviction. Add a constant (e.g., REGEX_CACHE_MAX =
1000) and update getCompiledRegex so that on cache hit you promote the key to
mark it recent (delete then set), and on inserting a new compiled RegExp you
check regexCache.size and evict the oldest entry (use
regexCache.keys().next().value) if size > REGEX_CACHE_MAX; alternatively,
replace the Map with an LRU cache library but ensure getCompiledRegex and the
regexCache symbol behavior are preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 375a444a-d336-4e88-a7aa-32d868fd97af
📒 Files selected for processing (22)
apps/backend/prisma/seed.tsapps/backend/src/app/api/latest/feature-flags/bootstrap/route.tsapps/backend/src/app/api/latest/feature-flags/evaluate/route.tsapps/backend/src/route-handlers/smart-response.tsxapps/dashboard/src/app/(main)/(protected)/projects/[projectId]/feature-flags/[flagId]/page-client.tsxapps/dashboard/src/app/(main)/(protected)/projects/[projectId]/feature-flags/page-client.tsxapps/e2e/tests/backend/endpoints/api/v1/feature-flags.test.tspackages/stack-shared/src/config/schema.tspackages/stack-shared/src/feature-flags/evaluator.tspackages/stack-shared/src/feature-flags/hashing.tspackages/stack-shared/src/feature-flags/types.tspackages/stack-shared/src/interface/client-interface.test.tspackages/stack-shared/src/interface/client-interface.tspackages/stack-shared/src/interface/crud/feature-flags.tspackages/template/src/index.tspackages/template/src/lib/hooks.tsxpackages/template/src/lib/stack-app/apps/implementations/client-app-feature-flags.test.tsxpackages/template/src/lib/stack-app/apps/implementations/client-app-impl.tspackages/template/src/lib/stack-app/apps/index.tspackages/template/src/lib/stack-app/apps/interfaces/client-app.tspackages/template/src/lib/stack-app/index.tssdks/spec/src/apps/client-app.spec.md
✅ Files skipped from review due to trivial changes (3)
- packages/template/src/lib/stack-app/apps/index.ts
- packages/template/src/lib/stack-app/index.ts
- packages/stack-shared/src/interface/client-interface.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/backend/prisma/seed.ts
- apps/dashboard/src/app/(main)/(protected)/projects/[projectId]/feature-flags/page-client.tsx
- packages/stack-shared/src/interface/crud/feature-flags.ts
- apps/backend/src/app/api/latest/feature-flags/bootstrap/route.ts
- apps/e2e/tests/backend/endpoints/api/v1/feature-flags.test.ts
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
- Updated package versions in pnpm-lock.yaml, including `react-dom` and `lightningcss`. - Added new dependencies for the dashboard application, including various UI and utility libraries. - Enhanced the `GET` route for feature flags to support additional response types and improved validation. - Refactored the `POST` route to streamline user ID handling and ensure consistent behavior. - Updated tests to reflect changes in API behavior and ensure proper request handling.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/backend/src/app/api/latest/feature-flags/bootstrap/route.ts`:
- Around line 74-83: The current If-None-Match check treats
headers["if-none-match"] as a single string and calls includes(etag), which
misses comma-separated ETag lists; update the comparison in the route so you
parse headers["if-none-match"] into individual entity tags (split on commas,
trim whitespace and surrounding quotes, and handle the wildcard "*" case) and
then check if any parsed tag equals the current etag or version before returning
the 304 response; locate the logic around the if block that references
headers["if-none-match"], etag and version to implement this parsing and
comparison.
In `@apps/backend/src/app/api/latest/internal/config/override/`[level]/route.tsx:
- Around line 301-317: The current flow reads the entire override
(levelConfig.get), merges with override(), validates, then writes the whole
document (levelConfig.set), which is not atomic and can silently clobber
concurrent sibling updates; instead build a path-based partial update from
parsedConfig and apply it with a path/patch API on levelConfig (e.g., use the
service's patch/update-paths method or a set call that accepts dotted-path keys)
rather than reading oldConfig and calling levelConfig.set with the merged
document; keep using assertConfigValidBeforeWrite but validate the specific
path(s) or the resulting merged paths, then call the path-based update
(referencing levelConfig.get, override, assertConfigValidBeforeWrite,
levelConfig.set in your changes) so concurrent updates don't overwrite unrelated
properties.
In `@packages/stack-shared/src/feature-flags/evaluator.ts`:
- Around line 28-45: compareSemver currently falls back to lexicalCompare for
malformed versions which lets bad inputs pass semver_gt/semver_lt; update parse
inside compareSemver so it performs strict validation (core must be three
numeric segments and prerelease parsed deterministically) and if either operand
is invalid return NaN (or another sentinel) instead of calling lexicalCompare,
then update semver_gt and semver_lt to call compareSemver and return false when
the result is NaN (i.e., only return true when compareSemver yields a valid
numeric comparison); ensure you remove the lexicalCompare fallback and apply the
same validation change where compareSemver is duplicated/used around lines
149-158.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 251d880d-ffb7-44d8-adec-444271adce91
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
apps/backend/src/app/api/latest/feature-flags/bootstrap/route.tsapps/backend/src/app/api/latest/feature-flags/evaluate/route.tsapps/backend/src/app/api/latest/internal/config/override/[level]/route.tsxapps/e2e/tests/backend/endpoints/api/v1/feature-flags.test.tspackages/stack-shared/package.jsonpackages/stack-shared/src/feature-flags/evaluator.tspackages/stack-shared/src/feature-flags/types.tspackages/stack-shared/src/interface/client-interface.test.tspackages/stack-shared/src/interface/client-interface.ts
✅ Files skipped from review due to trivial changes (1)
- packages/stack-shared/package.json
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/stack-shared/src/interface/client-interface.ts
- packages/stack-shared/src/interface/client-interface.test.ts
- apps/backend/src/app/api/latest/feature-flags/evaluate/route.ts
- apps/e2e/tests/backend/endpoints/api/v1/feature-flags.test.ts
- Added a `hidden` metadata property to the `GET` route for feature flags to control visibility. - Refactored the `if-none-match` header handling in the `GET` route to improve cache validation using a new `parseIfNoneMatch` function. - Updated the `PATCH` route for configuration overrides to streamline the process by directly using the new override methods for branch, environment, and project configurations. - Improved user feedback in the dashboard by changing the alert message for failed feature flag creation attempts.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
apps/dashboard/src/app/(main)/(protected)/projects/[projectId]/feature-flags/page-client.tsx (3)
69-70: Add defensive fallback for potentially undefinedflags.If
config.featureFlags.flagsis undefined (e.g., a freshly initialized project), passingundefinedtotypedEntriescould behave unexpectedly. A defensive fallback keeps the component robust.🛡️ Proposed fix
- const flags = config.featureFlags.flags; + const flags = config.featureFlags.flags ?? {};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/app/`(main)/(protected)/projects/[projectId]/feature-flags/page-client.tsx around lines 69 - 70, config.featureFlags.flags may be undefined, so calling typedEntries(flags) can break; add a defensive fallback by ensuring flags is an object/empty map before calling typedEntries (e.g., set flags = config.featureFlags.flags ?? {}), then compute entries = typedEntries(flags) so typedEntries always receives a defined value; update the code around the variables flags and entries in page-client.tsx to use this fallback.
140-142: Consider using??for consistency with other nullish checks.Line 140 uses
||while line 142 uses??. With||, an empty string""fordef.keywould fall back toflagId, whereas??only falls back fornull/undefined. If empty keys shouldn't exist (enforced by validation), using??maintains consistent semantics. As per coding guidelines, prefer explicit null/undefined checks over boolean checks.♻️ Proposed fix
- title={def.key || flagId} + title={def.key ?? flagId}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/app/`(main)/(protected)/projects/[projectId]/feature-flags/page-client.tsx around lines 140 - 142, The title prop currently uses a boolean fallback (title={def.key || flagId}) which treats empty string as absent; change it to a nullish fallback (title={def.key ?? flagId}) to match the existing nullish check used for subtitle (def.type ?? "boolean") and keep semantics consistent for def.key and flagId; locate the title assignment in the component rendering where title, subtitle are set and replace the || operator with ?? for def.key.
181-181: Replace type cast with a type guard for safer type narrowing.The
as FlagTypecast bypasses type safety. A type guard validates the value at runtime and satisfies the type system without an unsafe cast. As per coding guidelines, avoid usingasto bypass the type system.♻️ Proposed fix
Add a helper near the
FlagTypedefinition:const FLAG_TYPES = new Set<FlagType>(["boolean", "multivariate", "json", "numeric", "string"]); function isFlagType(v: string): v is FlagType { return FLAG_TYPES.has(v as FlagType); }Then use it in the handler:
- <Select value={newType} onValueChange={(v) => setNewType(v as FlagType)}> + <Select value={newType} onValueChange={(v) => { if (isFlagType(v)) setNewType(v); }}>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/app/`(main)/(protected)/projects/[projectId]/feature-flags/page-client.tsx at line 181, Replace the unsafe cast in the Select onValueChange handler by adding a runtime type guard for FlagType: define FLAG_TYPES (Set of valid FlagType strings) and an isFlagType(v: string): v is FlagType function, then change the handler in Select (the component using newType and setNewType) to call isFlagType(v) and only call setNewType(v) when the guard returns true; this removes the use of "as FlagType" and preserves type safety for newType and setNewType.apps/backend/src/app/api/latest/internal/config/override/[level]/route.tsx (2)
260-264: Avoid double-validating the same config in PUT.You validate in
assertConfigValidBeforeWrite(Line 260) and then immediately validate again inwarnOnValidationFailure(Line 273) with the same payload. Consider removing or gating the second call to reduce redundant work and transient log noise.Also applies to: 273-277
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/src/app/api/latest/internal/config/override/`[level]/route.tsx around lines 260 - 264, The PUT handler is running duplicate validation: assertConfigValidBeforeWrite(levelConfig, {projectId,branchId,config: parsedConfig}) and then immediately calling warnOnValidationFailure with the same payload; remove the redundant work by reusing the first validation result or gating the second call—either have assertConfigValidBeforeWrite return its validation result and pass that into warnOnValidationFailure (so warnOnValidationFailure no longer re-runs validation), or only call warnOnValidationFailure when assertConfigValidBeforeWrite completes successfully (avoiding a second validation run). Ensure you update references to levelConfig, parsedConfig, assertConfigValidBeforeWrite, and warnOnValidationFailure accordingly.
59-62: Add inline comments explaining theanytypes, or refactor to use schema-derived types.The three instances of
config: anyin the validate and helper function signatures (lines 59, 89, and 215) lack justification. Per coding guidelines, useyup.InferType<typeof schema>or add a comment explaining whyanyis necessary here (e.g., because the config undergoes runtime validation and normalization with complex polymorphic transforms).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/src/app/api/latest/internal/config/override/`[level]/route.tsx around lines 59 - 62, The parameter types currently using `any` (e.g., the `options.config` in the validate callback and the helper functions like validateProjectConfigOverride and the other helper at line ~215) should be replaced with schema-derived types or documented why `any` is required: change `any` to `yup.InferType<typeof <YourSchemaName>>` (e.g., ProjectConfigOverrideSchema) so the validateProjectConfigOverride signature becomes typed from the schema, or if you cannot due to complex polymorphic transforms, add a concise inline comment above each function explaining that runtime validation/normalization makes static typing impractical and why `any` is used temporarily (include reference to the specific schema used). Ensure you update all three spots (the validate callback, validateProjectConfigOverride, and the helper at ~215) to use the same approach and reference the schema symbol name to keep types consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/stack-shared/src/feature-flags/evaluator.ts`:
- Around line 150-158: The toMs helper in the "before"/"after" branch currently
uses Date.parse on arbitrary strings which is implementation-dependent; update
the toMs function (used in the "before"/"after" case) to fail closed for string
inputs by only accepting Date objects or numeric epoch values, or if you must
support strings validate against a strict RFC3339/ISO8601 pattern first and only
then parse; return undefined for any non-matching strings so comparisons are
deterministic across runtimes.
- Around line 18-25: getDottedAttribute currently uses Reflect.get which walks
the prototype chain and can return inherited properties; change it to only read
own properties: when iterating parts inside getDottedAttribute, after confirming
cursor is non-null and typeof object, check
Object.prototype.hasOwnProperty.call(cursor, part) (or
Object.getOwnPropertyDescriptor) and if the property is not an own property
return undefined; if it is own, read the value via direct property access (e.g.,
(cursor as Record<string, unknown>)[part]) and continue. This ensures
getDottedAttribute resolves only explicitly provided fields and not inherited
ones.
---
Nitpick comments:
In `@apps/backend/src/app/api/latest/internal/config/override/`[level]/route.tsx:
- Around line 260-264: The PUT handler is running duplicate validation:
assertConfigValidBeforeWrite(levelConfig, {projectId,branchId,config:
parsedConfig}) and then immediately calling warnOnValidationFailure with the
same payload; remove the redundant work by reusing the first validation result
or gating the second call—either have assertConfigValidBeforeWrite return its
validation result and pass that into warnOnValidationFailure (so
warnOnValidationFailure no longer re-runs validation), or only call
warnOnValidationFailure when assertConfigValidBeforeWrite completes successfully
(avoiding a second validation run). Ensure you update references to levelConfig,
parsedConfig, assertConfigValidBeforeWrite, and warnOnValidationFailure
accordingly.
- Around line 59-62: The parameter types currently using `any` (e.g., the
`options.config` in the validate callback and the helper functions like
validateProjectConfigOverride and the other helper at line ~215) should be
replaced with schema-derived types or documented why `any` is required: change
`any` to `yup.InferType<typeof <YourSchemaName>>` (e.g.,
ProjectConfigOverrideSchema) so the validateProjectConfigOverride signature
becomes typed from the schema, or if you cannot due to complex polymorphic
transforms, add a concise inline comment above each function explaining that
runtime validation/normalization makes static typing impractical and why `any`
is used temporarily (include reference to the specific schema used). Ensure you
update all three spots (the validate callback, validateProjectConfigOverride,
and the helper at ~215) to use the same approach and reference the schema symbol
name to keep types consistent.
In
`@apps/dashboard/src/app/`(main)/(protected)/projects/[projectId]/feature-flags/page-client.tsx:
- Around line 69-70: config.featureFlags.flags may be undefined, so calling
typedEntries(flags) can break; add a defensive fallback by ensuring flags is an
object/empty map before calling typedEntries (e.g., set flags =
config.featureFlags.flags ?? {}), then compute entries = typedEntries(flags) so
typedEntries always receives a defined value; update the code around the
variables flags and entries in page-client.tsx to use this fallback.
- Around line 140-142: The title prop currently uses a boolean fallback
(title={def.key || flagId}) which treats empty string as absent; change it to a
nullish fallback (title={def.key ?? flagId}) to match the existing nullish check
used for subtitle (def.type ?? "boolean") and keep semantics consistent for
def.key and flagId; locate the title assignment in the component rendering where
title, subtitle are set and replace the || operator with ?? for def.key.
- Line 181: Replace the unsafe cast in the Select onValueChange handler by
adding a runtime type guard for FlagType: define FLAG_TYPES (Set of valid
FlagType strings) and an isFlagType(v: string): v is FlagType function, then
change the handler in Select (the component using newType and setNewType) to
call isFlagType(v) and only call setNewType(v) when the guard returns true; this
removes the use of "as FlagType" and preserves type safety for newType and
setNewType.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ff8cce1b-8187-41f7-ba38-dfea16d3f590
📒 Files selected for processing (4)
apps/backend/src/app/api/latest/feature-flags/bootstrap/route.tsapps/backend/src/app/api/latest/internal/config/override/[level]/route.tsxapps/dashboard/src/app/(main)/(protected)/projects/[projectId]/feature-flags/page-client.tsxpackages/stack-shared/src/feature-flags/evaluator.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/backend/src/app/api/latest/feature-flags/bootstrap/route.ts
Comment on lines
+150
to
+158
| case "before": | ||
| case "after": { | ||
| const toMs = (v: unknown) => { | ||
| if (v instanceof Date) return v.getTime(); | ||
| if (typeof v === "number") return v; | ||
| if (typeof v === "string") { | ||
| const ms = Date.parse(v); | ||
| return Number.isNaN(ms) ? undefined : ms; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Date.parse() determinism Node.js vs browser JavaScript differences implementation-defined
💡 Result:
Date.parse is not deterministic across JavaScript environments like Node.js and browsers due to its implementation-defined behavior for non-standard date string formats. The ECMAScript specification mandates parsing only for a specific simplified ISO 8601 format (e.g., "YYYY-MM-DDTHH:mm:ss.sssZ"). All other formats may use engine-specific heuristics, leading to different results or Invalid Date across engines. Key differences: - Both Node.js (V8 engine) and Chrome (V8) share the same lenient parser, often accepting ambiguous formats like MM/DD/YYYY as local time or handling edge cases (e.g., "2015/03/31" as local midnight vs "2015-03-31" as UTC midnight). - Firefox (SpiderMonkey) may reject more strings as Invalid Date or interpret differently (e.g., stricter on day-first formats like "29/06/2024"). - Safari has historically varied, sometimes rejecting space-separated ISO-like strings. - Examples: new Date("2024-06-29 14:30:00") (space instead of T) works in Chrome/Node.js/Firefox as local but may fail in older Safari. new Date("06/29/2024") is US-local in most but DD/MM/YYYY in some older Firefox. Node.js vs browsers: Minimal differences since Node.js uses V8 like Chrome. Subtle variations can arise from system timezone settings or V8 version differences, but primarily engine-driven. Pre-ES6, more variance existed; ES6+ standardized some ISO handling (absent offset = local time). Avoid Date.parse for non-ISO; use manual parsing, Date.UTC, or libraries like date-fns/Luxon. Temporal API (ES2026+) offers deterministic alternatives.
Citations:
- 1: https://dateandtimeconverter.com/guides/javascript-date-parsing
- 2: https://stackoverflow.com/questions/2587345/why-does-date-parse-give-incorrect-results
- 3: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/parse
- 4: new Date() gives time in UTC timezone instead of browser/system timezone nodejs/node#19480
- 5: https://www.eppendorf.com
- 6: https://www.js.org
- 7: https://stackoverflow.com/questions/30964943/is-there-any-workaround-for-broken-v8-date-parser
- 8: https://stackoverflow.com/questions/30199130/different-result-for-yyyy-mm-dd-and-yyyy-mm-dd-in-javascript-when-passed-to-new/30199268
🌐 Web query:
JavaScript Date.parse RFC 3339 ISO 8601 format Node.js browser compatibility implementation-defined
💡 Result:
JavaScript's Date.parse has implementation-defined behavior for most date string formats, including those beyond the strict ECMAScript "Date Time String Format" (a profile of ISO 8601 extended format similar to RFC 3339). Only strings exactly matching this format are required to parse consistently across engines: YYYY-MM-DDTHH:mm:ss.sssZ (with optional reduced precision, optional ±HH:mm offset instead of Z). RFC 3339 and full ISO 8601 are broader; Date.parse must accept the spec format but may fall back to engine-specific parsing for others, leading to inconsistencies. Examples: - Date-only YYYY-MM-DD parses as UTC (spec requirement, differs from ISO 8601 local interpretation). - Variations like lowercase 't/z', out-of-bounds days (e.g., "2018-02-30"), extra fraction digits, or missing colons in offsets often parse differently: Chrome/Edge/Safari may normalize/fix, Firefox rejects more strictly. Browser compatibility (ISO 8601 basic support since ~2010-2015, full across modern browsers): - Chrome 6+, Firefox 4+, Safari 5.1+, Edge 12+, Opera 10+. Node.js (V8-based) matches Chrome behavior. No changes in ES2026+; Temporal API introduced as reliable alternative, but Date.parse remains implementation-defined. For cross-environment safety, use strict spec format or libraries/Temporal. Manual parsing recommended for RFC 3339 edge cases.
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date...
- 2: https://github.com/tc39/proposal-uniform-interchange-date-parsing
- 3: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/parse
- 4: https://caniuse.com/mdn-javascript_builtins_date_parse_iso_8601
Make date comparisons use a deterministic string format.
Date.parse() accepts implementation-defined formats beyond ISO 8601. Because this evaluator runs in both the backend and SDKs, a non-ISO input can produce different "before" / "after" results across runtimes. Please fail closed unless the string is in a strict format (such as RFC 3339/ISO 8601), or restrict this operator to Date / epoch-number inputs only.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/stack-shared/src/feature-flags/evaluator.ts` around lines 150 - 158,
The toMs helper in the "before"/"after" branch currently uses Date.parse on
arbitrary strings which is implementation-dependent; update the toMs function
(used in the "before"/"after" case) to fail closed for string inputs by only
accepting Date objects or numeric epoch values, or if you must support strings
validate against a strict RFC3339/ISO8601 pattern first and only then parse;
return undefined for any non-matching strings so comparisons are deterministic
across runtimes.
- Set React and React-DOM to version 19.2.3 in package.json. - Updated pnpm-lock.yaml to reflect the new versions for React and React-DOM, ensuring consistency across dependencies.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Around line 101-102: package.json pins runtime "react" and "react-dom" to
19.2.3 while the workspace override forces `@types/react` and `@types/react-dom` to
^18.2.0, causing a major-version mismatch; update the workspace overrides so the
dev/type packages match React 19 (e.g., set `@types/react` and `@types/react-dom` to
the correct major for React 19), or alternatively align the runtime to React 18
if you must keep the existing types—modify the overrides entry for
"@types/react" and "@types/react-dom" (and any related peerDependency entries)
to the matching major version to ensure type/runtime parity across the
workspace.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: be85bd6e-4487-4230-9d4f-3dc8e9747dfc
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (1)
package.json
Comment on lines
+101
to
+102
| "react": "19.2.3", | ||
| "react-dom": "19.2.3", |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
import json, pathlib
root = json.loads(pathlib.Path("package.json").read_text())
print("Root versions:")
print(" devDependencies.react =", root.get("devDependencies", {}).get("react"))
print(" devDependencies.react-dom =", root.get("devDependencies", {}).get("react-dom"))
print(" pnpm.overrides.@types/react =", root.get("pnpm", {}).get("overrides", {}).get("@types/react"))
print(" pnpm.overrides.@types/react-dom =", root.get("pnpm", {}).get("overrides", {}).get("@types/react-dom"))
print("\nWorkspace package.json files with React/type entries:")
for p in pathlib.Path(".").rglob("package.json"):
if "node_modules" in p.parts:
continue
data = json.loads(p.read_text())
found = []
for section in ("dependencies", "devDependencies", "peerDependencies", "optionalDependencies"):
deps = data.get(section, {})
for key in ("react", "react-dom", "@types/react", "@types/react-dom"):
if key in deps:
found.append((section, key, deps[key]))
if found:
print(f"- {p}")
for section, key, val in found:
print(f" {section}.{key} = {val}")
PYRepository: stack-auth/stack-auth
Length of output: 5396
Align React runtime and React type majors.
Lines 101-102 pin runtime to React 19.2.3, but lines 113-114 override @types/react and @types/react-dom to ^18.2.0. This major version mismatch between runtime and types can break or hide React 19 typings across the entire workspace, even for packages that declare higher type versions locally, since pnpm overrides take precedence workspace-wide.
Suggested fix
"pnpm": {
"overrides": {
- "@types/react": "^18.2.0",
- "@types/react-dom": "^18.2.0"
+ "@types/react": "^19.0.0",
+ "@types/react-dom": "^19.0.0"
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` around lines 101 - 102, package.json pins runtime "react" and
"react-dom" to 19.2.3 while the workspace override forces `@types/react` and
`@types/react-dom` to ^18.2.0, causing a major-version mismatch; update the
workspace overrides so the dev/type packages match React 19 (e.g., set
`@types/react` and `@types/react-dom` to the correct major for React 19), or
alternatively align the runtime to React 18 if you must keep the existing
types—modify the overrides entry for "@types/react" and "@types/react-dom" (and
any related peerDependency entries) to the matching major version to ensure
type/runtime parity across the workspace.
…ck.yaml - Deleted React and React-DOM version specifications from package.json. - Updated pnpm-lock.yaml to remove references to React and React-DOM, ensuring a clean dependency list.
- Introduced verification for primary email in the feature flag evaluation context to ensure only verified emails are used. - Updated the evaluation logic to handle distinct IDs more flexibly, allowing for better fallback mechanisms. - Improved the PATCH route for configuration overrides by implementing a new method to merge old and new configurations, ensuring validation before writing. - Added comprehensive tests to validate the new feature flag evaluation scenarios, including handling of unverified emails and dependency cycles.
Comment on lines
+318
to
+322
| return this.createError({ | ||
| message: "Feature flag regex conditions must use a string value", | ||
| path: `${this.path}.value`, | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
not_in / in condition values are not type-validated
The condition value is declared as yupMixed(), so the schema accepts any JSON value for every operator. For the not_in operator specifically, applyOperator is:
case "not_in": { return !(Array.isArray(expected) && expected.includes(actual)); }When expected is not an array (e.g. someone types "admin" as a plain string instead of ["admin"]), Array.isArray(expected) is false so the whole expression becomes !false === true, meaning every user matches the rule unconditionally. This silently turns an exclusion rule into an "always serve this variant" rule with no error at save time.
The same inverted failure applies to in: a non-array value makes it always-false, silently blocking everyone.
Consider adding a yup.when("operator", …) refinement that enforces Array.isArray(value) when operator is in or not_in.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/stack-shared/src/config/schema.ts
Line: 318-322
Comment:
**`not_in` / `in` condition values are not type-validated**
The condition `value` is declared as `yupMixed()`, so the schema accepts any JSON value for every operator. For the `not_in` operator specifically, `applyOperator` is:
```ts
case "not_in": { return !(Array.isArray(expected) && expected.includes(actual)); }
```
When `expected` is not an array (e.g. someone types `"admin"` as a plain string instead of `["admin"]`), `Array.isArray(expected)` is `false` so the whole expression becomes `!false === true`, meaning **every user matches the rule unconditionally**. This silently turns an exclusion rule into an "always serve this variant" rule with no error at save time.
The same inverted failure applies to `in`: a non-array value makes it always-false, silently blocking everyone.
Consider adding a `yup.when("operator", …)` refinement that enforces `Array.isArray(value)` when `operator` is `in` or `not_in`.
How can I resolve this? If you propose a fix, please make it concise.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
First-cut feature flag system: definitions in branch config, a pure deterministic evaluator shared between backend and SDK-facing types, bootstrap/evaluate routes, dashboard CRUD, SDK helpers, seed data, and an example demo page.
This also includes the follow-up review fixes around evaluator correctness, config validation, client-side context spoofing, stable bootstrap caching, dashboard save behavior, and SDK helper ergonomics.
Walkthrough
Data model —
packages/stack-shared/src/config/schema.tsbranchFeatureFlagsSchemaplugs intobranchConfigSchemaasfeatureFlags. Definitions live in config so they inherit branch/environment overrides for free; evaluation data such as exposures, audit logs, cohorts, and experiment events can live in dedicated tables later.Each flag has an opaque config id plus a user-facing
key, atype(boolean | multivariate | json | numeric | string),variants, priority-orderedrules,defaultVariantKey, and operational controls likeenabled,killSwitch,dependsOn, andholdoutId.Validation now enforces:
variantKeyorvariantWeightsdefaultVariantKey,rules.*.variantKey, andrules.*.variantWeightsonly reference existing variantsEvaluator —
packages/stack-shared/src/feature-flags/evaluator.tsPure, no IO.
evaluateFlag(flagId, config, ctx)returns{ variantKey, value, reason, ruleId? }.Resolution order is:
missing -> kill_switch -> disabled -> dep_unmet/cycle -> holdout -> matched_rule -> default.Rules sort by
prioritydescending with id tiebreaks. Conditions supporteq/neq/contains/regex/gt/in/before/semver_*/in_cohort/...over dotted attribute paths likeuser.email,team.plan, orcontext.country.Review fixes here:
dependsOnuses shared truthiness, including non-empty JSON objects/arrayssemver_*comparisons now handle prerelease precedence and ignore build metadataMapinternally before serializationHashing —
packages/stack-shared/src/feature-flags/hashing.tsMurmurHash3 32-bit. Salt convention is
${flagKey}.${ruleId}.${rolloutSeed}so the same user can land in different buckets across flags.The bucket separator is now a named
"\\x01"constant instead of an invisible literal control character. That keeps assignments stable while making the hashing contract visible in review/diffs.Routes —
apps/backend/src/app/api/latest/feature-flags/GET /bootstrapreturns full flag config for the tenancy. SDKs can cache and evaluate locally.ownerUserIdis stripped. The response includes a stable murmur3version, anETag, and supportsIf-None-Matchrevalidation with304.POST /evaluateperforms server-side eval.distinct_idfalls back to the authenticated user id.Important auth behavior: client-key requests no longer trust caller-supplied targeting context (
body.user,body.team,body.context,body.cohorts). Browser clients can still passdistinct_idwhen using the raw endpoint, but the SDK helpers use the authenticated Stack user id. Arbitrary targeting context is only accepted from server/admin access. This prevents a browser caller from spoofinguser.emailto match internal targeting rules.apps/backend/src/route-handlers/smart-response.tsxwas also adjusted so204/304responses are emitted without bodies, which is required by HTTP and by the platformResponseconstructor.SDK helpers —
packages/template+sdks/specThe client SDK now has app methods and provider hooks for evaluate-by-key:
app.getFeatureFlag(key)app.getFeatureFlags(keys)app.useFeatureFlag(key)app.useFeatureFlags(keys)useFeatureFlag()anduseFeatureFlags()The helpers call
/api/v1/feature-flags/evaluate, return camelCase result objects, batch multiple keys, and only sendflag_keys. They intentionally do not accept arbitrary client targeting context or caller-provided bucketing ids, matching the route-level anti-spoofing behavior. Bucketing comes from the authenticated Stack user id, including Stack anonymous users when the app creates one.sdks/spec/src/apps/client-app.spec.mddocuments the helper contract so future SDK implementations match the JS SDK behavior.Dashboard + seed
New "Feature Flags" app under
/projects/[projectId]/feature-flags, with list + detail editor going throughuseUpdateConfigrather than a separate API.Dashboard review fixes:
updateConfig()returns successSeed adds three demo flags:
new-checkout— 25% boolean rolloutpricing-experiment— 50/25/25 multivariate splitinternal-tools— anchored@stack-auth.com$email rule for trusted/server-side contextDemo page
examples/demo/src/app/feature-flags-demo/page.tsxvalidates required public env vars up front and strips trailing slashes fromNEXT_PUBLIC_STACK_API_URL, so setup mistakes fail with clear errors instead of producingundefined/api/...requests.API / SDK usage
Client-side SDK use:
React hook use:
Trusted server/admin callers can still use the raw evaluate endpoint with richer targeting context:
Notes:
key, not config id. The route resolves viafindFlagIdByKey.reasoncomes back on every result for debugging and future exposure analytics./bootstrapis wired up but no SDK consumes it yet; a future SDK can use it for local evaluation and cache revalidation.Natural follow-up out of scope here: a Vercel Flags adapter that bootstraps at edge time. The evaluator + hashing are designed so local SDK evaluation matches server evaluation.
Test plan
pnpm typecheckpnpm test run packages/stack-shared/src/feature-flags/evaluator.tspnpm test run apps/e2e/tests/backend/endpoints/api/v1/feature-flags.test.tspnpm test run packages/stack-shared/src/interface/client-interface.test.tspnpm -C packages/template typecheckpnpm -C packages/stack-shared typecheckpnpm -C packages/template lintpnpm -C packages/stack-shared lintownerUserIdETag/If-None-Matchrevalidation returns304distinct_idomission falls back to authenticated user idDashboard page for manual smoke testing:
http://localhost:8101/projects/-selector-/feature-flagsSummary by CodeRabbit
New Features
Tests