feature flags#1010
Open
goldflag wants to merge 8 commits into
Open
Conversation
|
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 an end-to-end feature-flag system: DB migration and schema, server-side evaluation and CRUD APIs with validation, client admin UI and typed endpoints/hooks, analytics-script visitor evaluation and tracker exposure events, ClickHouse/tracking schema updates, and multi-locale message updates. ChangesFeature Flags System
Sequence Diagram(s)sequenceDiagram
participant Browser
participant AnalyticsScript
participant Server
participant Postgres
participant ClickHouse
Browser->>AnalyticsScript: Load tracking script
AnalyticsScript->>AnalyticsScript: Generate/load visitorId
AnalyticsScript->>Server: POST /sites/:siteId/feature-flags/evaluate (context)
Server->>Postgres: fetch flags for site
Server->>Server: evaluateFeatureFlagsForSite(context)
Server->>AnalyticsScript: return assignments
AnalyticsScript->>AnalyticsScript: store in config.featureFlags
Browser->>AnalyticsScript: window[ns].flag(key)
AnalyticsScript->>Server: track feature_flag_exposure event
Server->>ClickHouse: insert event with feature_flags
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server/drizzle/0006_first_logan.sql (1)
1-25:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse Drizzle push workflow instead of committing manual SQL migration artifacts.
This migration file conflicts with the repo’s established Postgres schema change process. Please apply this schema change via
npm run db:pushand keep schema state aligned through that flow rather than manual SQL migration files.Based on learnings: "In this repo (rybbit-io/rybbit), schema updates for PostgreSQL should be synced using
npm run db:push(Drizzle push) rather than adding/maintaining SQL migration files or manually issuingALTER TYPEstatements."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/drizzle/0006_first_logan.sql` around lines 1 - 25, The PR adds a manual SQL migration creating the "feature_flags" table and its constraints (e.g., CONSTRAINT "feature_flags_site_key_unique", "feature_flags_rollout_check", "feature_flags_type_check", FK "feature_flags_site_id_sites_site_id_fk") which conflicts with the repo’s Drizzle push workflow; instead remove this manual SQL artifact and apply the schema change through the Drizzle workflow by updating your Drizzle schema (model for feature_flags) and running npm run db:push so the table, indexes (feature_flags_site_idx) and constraints are generated and tracked by Drizzle rather than committing raw SQL migration files.server/src/analytics-script/config.test.ts (1)
115-164:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing mock for the feature flags fetch in failure scenarios.
When the tracking config fetch fails (lines 120-123) or throws (line 171),
fetchFeatureFlagsis still called afterward (per the implementation). These tests only mock onefetchresponse, so the second call will either returnundefinedor throw unexpectedly. Add a second mock response for the feature flags endpoint:Proposed fix for "API call fails" test
// Mock failed API response - (global.fetch as any).mockResolvedValueOnce({ - ok: false, - status: 404, - }); + (global.fetch as any) + .mockResolvedValueOnce({ + ok: false, + status: 404, + }) + .mockResolvedValueOnce({ + ok: false, + });Proposed fix for "network error" test
// Mock network error - (global.fetch as any).mockRejectedValueOnce(new Error("Network error")); + (global.fetch as any) + .mockRejectedValueOnce(new Error("Network error")) + .mockResolvedValueOnce({ ok: false });Also applies to: 166-212
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/analytics-script/config.test.ts` around lines 115 - 164, The test for parseScriptConfig does not mock the second fetch call to fetchFeatureFlags, causing unexpected behavior; update the "should use defaults when API call fails" and the analogous "network error" test to add a second global.fetch mockResolvedValueOnce that returns a successful feature-flags response (e.g., { ok: true, json: async () => ({}) }) after the first failing/throwing response so fetchFeatureFlags receives a valid response; reference parseScriptConfig and fetchFeatureFlags to locate where the dual fetch calls occur and ensure both tests add the extra mockResolvedValueOnce for the feature flags call.
🧹 Nitpick comments (3)
server/src/analytics-script/tracking.ts (1)
22-22: 💤 Low value
exposedFeatureFlagsSet can grow unbounded in long-running SPAs.The Set is never cleared, so every unique
key:version:valuecombination accumulates indefinitely. In long-lived sessions with frequent flag changes, this could cause memory growth. Consider clearing it on page changes or limiting its size.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/analytics-script/tracking.ts` at line 22, The exposedFeatureFlags Set on the Tracking class (exposedFeatureFlags) is unbounded and can grow indefinitely; modify the code that manages exposedFeatureFlags to prevent unbounded growth by either (a) clearing it on navigation/page-change events (listen for popstate/history changes or framework router events and call exposedFeatureFlags.clear()), or (b) enforce a fixed capacity (implement an eviction strategy such as simple FIFO/LRU: when adding in the method that constructs the "key:version:value" string, if exposedFeatureFlags.size >= MAX_FLAGS then remove the oldest entry before adding the new one). Update the add/check logic where exposedFeatureFlags is used so it uses the new clear-on-navigation or capped-set behavior.server/src/analytics-script/config.ts (1)
47-84: ⚡ Quick winConsider adding a timeout to the feature flags fetch.
fetchFeatureFlagshas no timeout, so a slow or hanging server could delay script initialization indefinitely. AnAbortControllerwith a short timeout (e.g., 2-3 seconds) would prevent blocking the page while still allowing best-effort evaluation.Proposed fix
async function fetchFeatureFlags( analyticsHost: string, siteId: string, namespace: string, visitorId: string ): Promise<ScriptConfig["featureFlags"]> { try { const url = new URL(window.location.href); + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 3000); + const response = await fetch(`${analyticsHost}/site/${siteId}/feature-flags/evaluate`, { method: "POST", headers: { "Content-Type": "application/json", }, credentials: "omit", + signal: controller.signal, body: JSON.stringify({ // ... }), }); + clearTimeout(timeoutId); if (!response.ok) { return {}; } // ... } catch (e) { return {}; } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/analytics-script/config.ts` around lines 47 - 84, fetchFeatureFlags currently can hang waiting for fetch; use an AbortController with a short timeout (e.g., 2500ms) to cancel the request and avoid blocking script init: create an AbortController, pass controller.signal to fetch, start a timer that calls controller.abort() after ~2500ms, and clear the timer in finally; ensure the existing catch (which returns {}) still handles the aborted request gracefully. Reference the fetchFeatureFlags function, the fetch call, and the catch/finally block when making these changes.server/src/analytics-script/index.ts (1)
54-60: ⚡ Quick winTighten queued argument typing (
any[]→ safer typed/unknown args).Using
any[]here bypasses strict-mode guarantees on the public API queue path.As per coding guidelines: `**/*.{ts,tsx}`: Use TypeScript with strict typing throughout both client and server.Suggested fix
- type QueueEntry = [string, any[]]; + type QueueEntry = [string, unknown[]]; const earlyQueue: QueueEntry[] = []; const queueMethod = (method: string) => - (...args: any[]) => { + (...args: unknown[]) => { earlyQueue.push([method, args]); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/analytics-script/index.ts` around lines 54 - 60, The tuple type QueueEntry and queueMethod currently use any[] which bypasses strict typing; change the second element from any[] to unknown[] (or a narrower union of allowed argument types) and update usages that read earlyQueue/QueueEntry to properly narrow or validate those unknowns (e.g., where entries are replayed/consumed, add type guards or casts). Specifically update the type alias QueueEntry, the earlyQueue declaration, and the queueMethod signature so it returns (...args: unknown[]) => void, and fix downstream consumers to handle/validate unknown[] instead of assuming any.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@client/messages/zh.json`:
- Line 10: The translation key "+R0VMp" currently has an empty value and must be
populated with the correct Chinese translation (or a temporary Chinese fallback
such as the English source) so the UI won't render blank; locate the key
"+R0VMp" in client/messages/zh.json and replace the empty string with the proper
translated text, and then scan other changed locale keys (the other empty
entries flagged in the review) in client/messages/*.json to ensure each key has
a non-empty zh value and matches the same keys used in other locales.
In `@client/src/api/analytics/hooks/featureFlags/useFeatureFlags.ts`:
- Around line 22-55: The mutation hooks call
createFeatureFlag/updateFeatureFlag/deleteFeatureFlag with an unguarded site
from useStore, which can be undefined during hydration; update
useCreateFeatureFlag, useUpdateFeatureFlag and useDeleteFeatureFlag to validate
site before calling the API by checking if (!site) and returning a rejected
Promise or throwing an Error inside the mutationFn (e.g., in
useUpdateFeatureFlag's mutationFn before calling updateFeatureFlag(site,...)),
so the mutation fails fast with a clear error instead of hitting
/sites/undefined/*; keep the existing onSuccess invalidation logic unchanged.
In `@client/src/app/`[site]/feature-flags/page.tsx:
- Around line 740-745: The "sessions" label is hardcoded in the span that
renders {stat.sessions.toLocaleString()} sessions; replace the literal with a
localized string using next-intl's useTranslations() (e.g., call const t =
useTranslations('FeatureFlags') at the top of the component in
client/src/app/[site]/feature-flags/page.tsx and render t('sessions') instead of
the hardcoded word), update the messages/ FeatureFlags translation JSON files to
include the "sessions" key (and plural forms if needed), and ensure the span
concatenates/localizes properly (e.g., use t('sessions', { count: stat.sessions
}) or format as required) so the label is translatable.
- Around line 32-33: The page is rendering hardcoded UI strings ("(empty)" and
"sessions")—use the existing next-intl extraction pattern instead of raw text:
obtain a translator (e.g., via useExtracted/translated messages or a t function
used in this file) and replace the literal "(empty)" and "sessions" usages with
localized calls (e.g., t('featureFlags.empty') and t('featureFlags.sessions'))
so they are extracted by the extractor; keep the useExtracted pattern already
imported and referenced in this file (page.tsx / feature-flags render logic) and
add appropriate message keys to the translation catalog.
In `@server/src/analytics-script/config.ts`:
- Around line 16-29: getOrCreateVisitorId currently returns a new
createVisitorId() whenever localStorage access throws, causing visitor IDs to
change on every page load; modify getOrCreateVisitorId to, on localStorage
failure, attempt to read/write the same key from sessionStorage and only if
sessionStorage is unavailable fall back to an in-memory (module-scoped) cached
id so the id is stable for the page session; update references to
localStorage.getItem/setItem to use this fallback sequence and ensure
createVisitorId() is only called once per session when no persisted value
exists.
In `@server/src/analytics-script/index.ts`:
- Line 223: The onReady invoker currently calls the consumer callback directly
(onReady: (callback: (api: RybbitAPI) => void) =>
callback(window[config.namespace])) which lets any exception in user code
propagate and break the tracker; change onReady to call the callback inside a
try/catch (and optionally schedule it asynchronously via setTimeout or
Promise.resolve().then(...)) so exceptions are caught and logged (e.g.,
console.error) and do not bubble up, using the same RybbitAPI instance from
window[config.namespace].
In `@server/src/analytics-script/tracking.ts`:
- Around line 70-93: refreshFeatureFlags can race when multiple calls overlap
and later responses overwrite config.featureFlags; modify refreshFeatureFlags
(and any caller state like identify()/clearUserId()) to track a unique request
token or use an AbortController so only the latest request updates
config.featureFlags: generate a monotonic requestId before fetch (or create an
AbortController and store it on the instance), attach it to the pending request,
and when the response arrives verify the requestId matches the current token (or
that the controller wasn't aborted) before assigning this.config.featureFlags;
ensure aborted/stale responses are ignored and any prior controller is aborted
when starting a new fetch.
In `@server/src/api/featureFlags/index.ts`:
- Around line 197-213: The current flow reads version via
db.query.featureFlags.findFirst into updateData.version, increments it in
memory, then writes—this is racy; instead perform an atomic DB update that
increments the stored version and returns the updated row (or use an optimistic
concurrency check): replace the two-step read+increment+update with a single
update against featureFlags that either (a) sets version = version + 1 in the DB
and returns the row (using the same .update(featureFlags).set(...).returning()
call), or (b) uses the current version in the WHERE clause
(eq(featureFlags.version, updateData.version)) so the update only succeeds if no
concurrent change happened and you can detect a 0-rows-updated and return a 409;
update code paths that reference updateData.version,
db.query.featureFlags.findFirst, and db.update(featureFlags).set(...)
accordingly so version is bumped atomically and concurrent updates are
detected/handled.
In `@server/src/index.ts`:
- Line 290: The POST route registering evaluateFeatureFlags currently bypasses
site access checks; update the registration to use the same site-scoped
middleware/grouping pattern used elsewhere in src/index.ts by wrapping or
registering the route inside the existing site plugin that runs resolveSiteId
and access checks (the same middleware that protects other site routes), so that
evaluateFeatureFlags is called only after resolveSiteId and the site access
verifier have run; locate the route registering evaluateFeatureFlags and move it
into the site plugin/group block or add the resolveSiteId + access-check
middleware to that route registration to enforce site-scoped access.
In `@server/src/services/featureFlags/evaluator.ts`:
- Around line 89-96: The regex branch in
server/src/services/featureFlags/evaluator.ts currently compiles untrusted
patterns per-request using new RegExp(value).test(actual) (inside the case
"regex" / expected.some callback), which is vulnerable to ReDoS; change the flow
to stop compiling at request time by prevalidating and precompiling patterns
when rules are created/loaded (reject patterns that safe-regex flags or compile
into a safe engine), cache those compiled patterns (or use a safe engine like
the re2 package to construct RE2 instances once) and then here (case "regex")
use the cached compiled object(s) to run .test(actual) instead of new
RegExp(value) — also ensure rule creation logic validates patterns with
safe-regex or rejects/limits dangerous patterns so runtime cannot receive
catastrophic regexes.
---
Outside diff comments:
In `@server/drizzle/0006_first_logan.sql`:
- Around line 1-25: The PR adds a manual SQL migration creating the
"feature_flags" table and its constraints (e.g., CONSTRAINT
"feature_flags_site_key_unique", "feature_flags_rollout_check",
"feature_flags_type_check", FK "feature_flags_site_id_sites_site_id_fk") which
conflicts with the repo’s Drizzle push workflow; instead remove this manual SQL
artifact and apply the schema change through the Drizzle workflow by updating
your Drizzle schema (model for feature_flags) and running npm run db:push so the
table, indexes (feature_flags_site_idx) and constraints are generated and
tracked by Drizzle rather than committing raw SQL migration files.
In `@server/src/analytics-script/config.test.ts`:
- Around line 115-164: The test for parseScriptConfig does not mock the second
fetch call to fetchFeatureFlags, causing unexpected behavior; update the "should
use defaults when API call fails" and the analogous "network error" test to add
a second global.fetch mockResolvedValueOnce that returns a successful
feature-flags response (e.g., { ok: true, json: async () => ({}) }) after the
first failing/throwing response so fetchFeatureFlags receives a valid response;
reference parseScriptConfig and fetchFeatureFlags to locate where the dual fetch
calls occur and ensure both tests add the extra mockResolvedValueOnce for the
feature flags call.
---
Nitpick comments:
In `@server/src/analytics-script/config.ts`:
- Around line 47-84: fetchFeatureFlags currently can hang waiting for fetch; use
an AbortController with a short timeout (e.g., 2500ms) to cancel the request and
avoid blocking script init: create an AbortController, pass controller.signal to
fetch, start a timer that calls controller.abort() after ~2500ms, and clear the
timer in finally; ensure the existing catch (which returns {}) still handles the
aborted request gracefully. Reference the fetchFeatureFlags function, the fetch
call, and the catch/finally block when making these changes.
In `@server/src/analytics-script/index.ts`:
- Around line 54-60: The tuple type QueueEntry and queueMethod currently use
any[] which bypasses strict typing; change the second element from any[] to
unknown[] (or a narrower union of allowed argument types) and update usages that
read earlyQueue/QueueEntry to properly narrow or validate those unknowns (e.g.,
where entries are replayed/consumed, add type guards or casts). Specifically
update the type alias QueueEntry, the earlyQueue declaration, and the
queueMethod signature so it returns (...args: unknown[]) => void, and fix
downstream consumers to handle/validate unknown[] instead of assuming any.
In `@server/src/analytics-script/tracking.ts`:
- Line 22: The exposedFeatureFlags Set on the Tracking class
(exposedFeatureFlags) is unbounded and can grow indefinitely; modify the code
that manages exposedFeatureFlags to prevent unbounded growth by either (a)
clearing it on navigation/page-change events (listen for popstate/history
changes or framework router events and call exposedFeatureFlags.clear()), or (b)
enforce a fixed capacity (implement an eviction strategy such as simple
FIFO/LRU: when adding in the method that constructs the "key:version:value"
string, if exposedFeatureFlags.size >= MAX_FLAGS then remove the oldest entry
before adding the new one). Update the add/check logic where exposedFeatureFlags
is used so it uses the new clear-on-navigation or capped-set behavior.
🪄 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: 4a19c00b-26bc-4053-9c39-ab763959035a
📒 Files selected for processing (44)
client/messages/cs.jsonclient/messages/de.jsonclient/messages/en.jsonclient/messages/es.jsonclient/messages/fr.jsonclient/messages/it.jsonclient/messages/ja.jsonclient/messages/ko.jsonclient/messages/pl.jsonclient/messages/pt.jsonclient/messages/uk.jsonclient/messages/zh.jsonclient/src/api/analytics/endpoints/featureFlags.tsclient/src/api/analytics/endpoints/index.tsclient/src/api/analytics/hooks/featureFlags/useFeatureFlags.tsclient/src/app/[site]/components/Sidebar/Sidebar.tsxclient/src/app/[site]/feature-flags/page.tsxclient/src/lib/siteRoute.tsserver/drizzle/0006_first_logan.sqlserver/drizzle/meta/0006_snapshot.jsonserver/drizzle/meta/_journal.jsonserver/public/script-full.jsserver/public/script.jsserver/src/analytics-script/config.test.tsserver/src/analytics-script/config.tsserver/src/analytics-script/index.tsserver/src/analytics-script/tracking.test.tsserver/src/analytics-script/tracking.tsserver/src/analytics-script/types.tsserver/src/api/analytics/utils/getFilterStatement.test.tsserver/src/api/analytics/utils/getFilterStatement.tsserver/src/api/analytics/utils/query-validation.tsserver/src/api/featureFlags/index.tsserver/src/api/featureFlags/schemas.tsserver/src/db/clickhouse/clickhouse.tsserver/src/db/postgres/schema.tsserver/src/index.tsserver/src/services/featureFlags/evaluator.test.tsserver/src/services/featureFlags/evaluator.tsserver/src/services/tracker/pageviewQueue.tsserver/src/services/tracker/trackEvent.tsserver/src/services/tracker/types.tsserver/src/services/tracker/utils.tsshared/src/filters.ts
Comment on lines
+22
to
+55
| export function useCreateFeatureFlag() { | ||
| const queryClient = useQueryClient(); | ||
| const { site } = useStore(); | ||
|
|
||
| return useMutation({ | ||
| mutationFn: (payload: FeatureFlagPayload) => createFeatureFlag(site, payload), | ||
| onSuccess: () => { | ||
| queryClient.invalidateQueries({ queryKey: ["feature-flags", site] }); | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| export function useUpdateFeatureFlag() { | ||
| const queryClient = useQueryClient(); | ||
| const { site } = useStore(); | ||
|
|
||
| return useMutation({ | ||
| mutationFn: ({ flagId, payload }: { flagId: number; payload: FeatureFlagUpdatePayload }) => | ||
| updateFeatureFlag(site, flagId, payload), | ||
| onSuccess: () => { | ||
| queryClient.invalidateQueries({ queryKey: ["feature-flags", site] }); | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| export function useDeleteFeatureFlag() { | ||
| const queryClient = useQueryClient(); | ||
| const { site } = useStore(); | ||
|
|
||
| return useMutation({ | ||
| mutationFn: (flagId: number) => deleteFeatureFlag(site, flagId), | ||
| onSuccess: () => { | ||
| queryClient.invalidateQueries({ queryKey: ["feature-flags", site] }); | ||
| }, |
Contributor
There was a problem hiding this comment.
Guard mutation calls when site context is missing.
Line [27], Line [39], and Line [52] call endpoints with site unguarded. If site is null/undefined during store hydration or route transitions, these hit invalid URLs (/sites/undefined/...).
Suggested fix
export function useCreateFeatureFlag() {
const queryClient = useQueryClient();
const { site } = useStore();
return useMutation({
- mutationFn: (payload: FeatureFlagPayload) => createFeatureFlag(site, payload),
+ mutationFn: (payload: FeatureFlagPayload) => {
+ if (site == null) throw new Error("Site is not selected");
+ return createFeatureFlag(site, payload);
+ },
onSuccess: () => {
queryClient.invalidateQueries({ queryKey: ["feature-flags", site] });
},
});
}
@@
return useMutation({
- mutationFn: ({ flagId, payload }: { flagId: number; payload: FeatureFlagUpdatePayload }) =>
- updateFeatureFlag(site, flagId, payload),
+ mutationFn: ({ flagId, payload }: { flagId: number; payload: FeatureFlagUpdatePayload }) => {
+ if (site == null) throw new Error("Site is not selected");
+ return updateFeatureFlag(site, flagId, payload);
+ },
onSuccess: () => {
queryClient.invalidateQueries({ queryKey: ["feature-flags", site] });
},
});
}
@@
return useMutation({
- mutationFn: (flagId: number) => deleteFeatureFlag(site, flagId),
+ mutationFn: (flagId: number) => {
+ if (site == null) throw new Error("Site is not selected");
+ return deleteFeatureFlag(site, flagId);
+ },
onSuccess: () => {
queryClient.invalidateQueries({ queryKey: ["feature-flags", site] });
},
});
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function useCreateFeatureFlag() { | |
| const queryClient = useQueryClient(); | |
| const { site } = useStore(); | |
| return useMutation({ | |
| mutationFn: (payload: FeatureFlagPayload) => createFeatureFlag(site, payload), | |
| onSuccess: () => { | |
| queryClient.invalidateQueries({ queryKey: ["feature-flags", site] }); | |
| }, | |
| }); | |
| } | |
| export function useUpdateFeatureFlag() { | |
| const queryClient = useQueryClient(); | |
| const { site } = useStore(); | |
| return useMutation({ | |
| mutationFn: ({ flagId, payload }: { flagId: number; payload: FeatureFlagUpdatePayload }) => | |
| updateFeatureFlag(site, flagId, payload), | |
| onSuccess: () => { | |
| queryClient.invalidateQueries({ queryKey: ["feature-flags", site] }); | |
| }, | |
| }); | |
| } | |
| export function useDeleteFeatureFlag() { | |
| const queryClient = useQueryClient(); | |
| const { site } = useStore(); | |
| return useMutation({ | |
| mutationFn: (flagId: number) => deleteFeatureFlag(site, flagId), | |
| onSuccess: () => { | |
| queryClient.invalidateQueries({ queryKey: ["feature-flags", site] }); | |
| }, | |
| export function useCreateFeatureFlag() { | |
| const queryClient = useQueryClient(); | |
| const { site } = useStore(); | |
| return useMutation({ | |
| mutationFn: (payload: FeatureFlagPayload) => { | |
| if (site == null) throw new Error("Site is not selected"); | |
| return createFeatureFlag(site, payload); | |
| }, | |
| onSuccess: () => { | |
| queryClient.invalidateQueries({ queryKey: ["feature-flags", site] }); | |
| }, | |
| }); | |
| } | |
| export function useUpdateFeatureFlag() { | |
| const queryClient = useQueryClient(); | |
| const { site } = useStore(); | |
| return useMutation({ | |
| mutationFn: ({ flagId, payload }: { flagId: number; payload: FeatureFlagUpdatePayload }) => { | |
| if (site == null) throw new Error("Site is not selected"); | |
| return updateFeatureFlag(site, flagId, payload); | |
| }, | |
| onSuccess: () => { | |
| queryClient.invalidateQueries({ queryKey: ["feature-flags", site] }); | |
| }, | |
| }); | |
| } | |
| export function useDeleteFeatureFlag() { | |
| const queryClient = useQueryClient(); | |
| const { site } = useStore(); | |
| return useMutation({ | |
| mutationFn: (flagId: number) => { | |
| if (site == null) throw new Error("Site is not selected"); | |
| return deleteFeatureFlag(site, flagId); | |
| }, | |
| onSuccess: () => { | |
| queryClient.invalidateQueries({ queryKey: ["feature-flags", site] }); | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@client/src/api/analytics/hooks/featureFlags/useFeatureFlags.ts` around lines
22 - 55, The mutation hooks call
createFeatureFlag/updateFeatureFlag/deleteFeatureFlag with an unguarded site
from useStore, which can be undefined during hydration; update
useCreateFeatureFlag, useUpdateFeatureFlag and useDeleteFeatureFlag to validate
site before calling the API by checking if (!site) and returning a rejected
Promise or throwing an Error inside the mutationFn (e.g., in
useUpdateFeatureFlag's mutationFn before calling updateFeatureFlag(site,...)),
so the mutation fails fast with a clear error instead of hitting
/sites/undefined/*; keep the existing onSuccess invalidation logic unchanged.
Comment on lines
+740
to
+745
| <div key={`${stat.flag_value}-${stat.sessions}`} className="flex items-center gap-2 text-xs"> | ||
| <span className="max-w-24 truncate font-mono text-neutral-700 dark:text-neutral-200"> | ||
| {stat.flag_value || "(empty)"} | ||
| </span> | ||
| <span className="text-neutral-500 dark:text-neutral-400">{stat.sessions.toLocaleString()} sessions</span> | ||
| </div> |
Contributor
There was a problem hiding this comment.
Localize the hardcoded “sessions” label.
Line [744] renders sessions as a raw English string, so this cell won't translate for non-English locales.
As per coding guidelines: client/src/**/*.{ts,tsx}: "Use next-intl's 'useTranslations()' hook for i18n; maintain translation files in messages/ directory."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@client/src/app/`[site]/feature-flags/page.tsx around lines 740 - 745, The
"sessions" label is hardcoded in the span that renders
{stat.sessions.toLocaleString()} sessions; replace the literal with a localized
string using next-intl's useTranslations() (e.g., call const t =
useTranslations('FeatureFlags') at the top of the component in
client/src/app/[site]/feature-flags/page.tsx and render t('sessions') instead of
the hardcoded word), update the messages/ FeatureFlags translation JSON files to
include the "sessions" key (and plural forms if needed), and ensure the span
concatenates/localizes properly (e.g., use t('sessions', { count: stat.sessions
}) or format as required) so the label is translatable.
Comment on lines
+16
to
+29
| function getOrCreateVisitorId(namespace: string): string { | ||
| const key = `${namespace}-visitor-id`; | ||
|
|
||
| try { | ||
| const stored = localStorage.getItem(key); | ||
| if (stored) return stored; | ||
|
|
||
| const visitorId = createVisitorId(); | ||
| localStorage.setItem(key, visitorId); | ||
| return visitorId; | ||
| } catch (e) { | ||
| return createVisitorId(); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Visitors without localStorage get a new ID on every page load.
When localStorage is unavailable (line 27), a fresh visitor ID is created each time. This breaks feature flag consistency for those users—they may see different flag assignments on every navigation. Consider falling back to a session-level ID stored in sessionStorage or in-memory, so at minimum the ID remains stable for the page session.
Proposed fix
+let inMemoryVisitorId: string | null = null;
+
function getOrCreateVisitorId(namespace: string): string {
const key = `${namespace}-visitor-id`;
try {
const stored = localStorage.getItem(key);
if (stored) return stored;
const visitorId = createVisitorId();
localStorage.setItem(key, visitorId);
return visitorId;
} catch (e) {
- return createVisitorId();
+ // localStorage unavailable - use in-memory fallback for session stability
+ if (!inMemoryVisitorId) {
+ inMemoryVisitorId = createVisitorId();
+ }
+ return inMemoryVisitorId;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function getOrCreateVisitorId(namespace: string): string { | |
| const key = `${namespace}-visitor-id`; | |
| try { | |
| const stored = localStorage.getItem(key); | |
| if (stored) return stored; | |
| const visitorId = createVisitorId(); | |
| localStorage.setItem(key, visitorId); | |
| return visitorId; | |
| } catch (e) { | |
| return createVisitorId(); | |
| } | |
| } | |
| let inMemoryVisitorId: string | null = null; | |
| function getOrCreateVisitorId(namespace: string): string { | |
| const key = `${namespace}-visitor-id`; | |
| try { | |
| const stored = localStorage.getItem(key); | |
| if (stored) return stored; | |
| const visitorId = createVisitorId(); | |
| localStorage.setItem(key, visitorId); | |
| return visitorId; | |
| } catch (e) { | |
| // localStorage unavailable - use in-memory fallback for session stability | |
| if (!inMemoryVisitorId) { | |
| inMemoryVisitorId = createVisitorId(); | |
| } | |
| return inMemoryVisitorId; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/src/analytics-script/config.ts` around lines 16 - 29,
getOrCreateVisitorId currently returns a new createVisitorId() whenever
localStorage access throws, causing visitor IDs to change on every page load;
modify getOrCreateVisitorId to, on localStorage failure, attempt to read/write
the same key from sessionStorage and only if sessionStorage is unavailable fall
back to an in-memory (module-scoped) cached id so the id is stable for the page
session; update references to localStorage.getItem/setItem to use this fallback
sequence and ensure createVisitorId() is only called once per session when no
persisted value exists.
| flagPayload: <T = unknown>(key: string, fallback?: T) => tracker.getFeatureFlagPayload<T>(key, fallback), | ||
| flags: () => tracker.getFeatureFlags(), | ||
| flagPayloads: () => tracker.getFeatureFlagPayloads(), | ||
| onReady: (callback: (api: RybbitAPI) => void) => callback(window[config.namespace]), |
Contributor
There was a problem hiding this comment.
Guard onReady callback execution to avoid consumer exceptions breaking the tracker.
onReady executes arbitrary user callback code directly; an exception there can break downstream script flow. Wrap it to isolate failures.
Suggested fix
- onReady: (callback: (api: RybbitAPI) => void) => callback(window[config.namespace]),
+ onReady: (callback: (api: RybbitAPI) => void) => {
+ try {
+ callback(window[config.namespace]);
+ } catch (error) {
+ console.error("onReady callback failed:", error);
+ }
+ },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| onReady: (callback: (api: RybbitAPI) => void) => callback(window[config.namespace]), | |
| onReady: (callback: (api: RybbitAPI) => void) => { | |
| try { | |
| callback(window[config.namespace]); | |
| } catch (error) { | |
| console.error("onReady callback failed:", error); | |
| } | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/src/analytics-script/index.ts` at line 223, The onReady invoker
currently calls the consumer callback directly (onReady: (callback: (api:
RybbitAPI) => void) => callback(window[config.namespace])) which lets any
exception in user code propagate and break the tracker; change onReady to call
the callback inside a try/catch (and optionally schedule it asynchronously via
setTimeout or Promise.resolve().then(...)) so exceptions are caught and logged
(e.g., console.error) and do not bubble up, using the same RybbitAPI instance
from window[config.namespace].
Comment on lines
+70
to
+93
| private async refreshFeatureFlags(): Promise<void> { | ||
| try { | ||
| const response = await fetch(`${this.config.analyticsHost}/site/${this.config.siteId}/feature-flags/evaluate`, { | ||
| method: "POST", | ||
| headers: { | ||
| "Content-Type": "application/json", | ||
| }, | ||
| body: JSON.stringify({ | ||
| anonymousId: this.config.visitorId, | ||
| identifiedUserId: this.customUserId || undefined, | ||
| ...this.getCurrentUrlContext(), | ||
| }), | ||
| mode: "cors", | ||
| credentials: "omit", | ||
| keepalive: true, | ||
| }); | ||
|
|
||
| if (!response.ok) return; | ||
| const data = await response.json(); | ||
| this.config.featureFlags = data?.flags && typeof data.flags === "object" ? data.flags : {}; | ||
| } catch (e) { | ||
| // Feature flag refresh is best-effort and should never affect analytics collection. | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Concurrent refreshFeatureFlags calls can cause a race condition.
If refreshFeatureFlags is called rapidly (e.g., identify() then clearUserId() in quick succession), responses may arrive out of order and overwrite config.featureFlags incorrectly. Consider using a request ID or aborting stale requests:
Proposed fix using request ID
+ private latestFlagRequestId = 0;
+
private async refreshFeatureFlags(): Promise<void> {
+ const requestId = ++this.latestFlagRequestId;
try {
const response = await fetch(`${this.config.analyticsHost}/site/${this.config.siteId}/feature-flags/evaluate`, {
// ...
});
if (!response.ok) return;
+ if (requestId !== this.latestFlagRequestId) return; // Stale response
const data = await response.json();
this.config.featureFlags = data?.flags && typeof data.flags === "object" ? data.flags : {};
} catch (e) {
// Feature flag refresh is best-effort and should never affect analytics collection.
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/src/analytics-script/tracking.ts` around lines 70 - 93,
refreshFeatureFlags can race when multiple calls overlap and later responses
overwrite config.featureFlags; modify refreshFeatureFlags (and any caller state
like identify()/clearUserId()) to track a unique request token or use an
AbortController so only the latest request updates config.featureFlags: generate
a monotonic requestId before fetch (or create an AbortController and store it on
the instance), attach it to the pending request, and when the response arrives
verify the requestId matches the current token (or that the controller wasn't
aborted) before assigning this.config.featureFlags; ensure aborted/stale
responses are ignored and any prior controller is aborted when starting a new
fetch.
…time types This commit adds support for condition sets in feature flags, allowing for more granular control over feature rollout. It introduces a new runtime type to specify whether a feature flag is applicable on the client, server, or both. Additionally, the database schema is updated to accommodate these changes, ensuring that the feature flag system is more flexible and robust.
…d filter management This commit updates the language files for Czech, German, English, Spanish, French, Italian, and Japanese, adding new keys and translations related to API endpoints, filters, and error messages. The changes improve the user experience by providing clearer instructions and feedback in various languages, ensuring consistency across the application.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/src/api/featureFlags/schemas.ts`:
- Around line 163-166: The current check using hasConditionSetVariants lets a
flag pass when only some conditionSets define their own variants; change the
validation so that if any conditionSet defines variants (compute e.g.
hasConditionSetWithVariants = conditionSets.some(cs => (cs.variants?.length ??
0) > 0)), then require the top-level variants array to have at least two
entries; update the conditional that calls ctx.addIssue to use this new
predicate (instead of the old hasConditionSetVariants) and ensure the error
message reflects that top-level variants must be multivariate when any condition
set relies on the fallback.
- Around line 240-246: The validator currently only rejects
conditionSet.rolloutPercentage for multivariate sets when setVariants.length >
0; change the check so any conditionSet.rolloutPercentage !== undefined triggers
ctx.addIssue when flagType === "multivariate" (remove the setVariants.length > 0
requirement). Update the if guard around the ctx.addIssue call that references
conditionSet.rolloutPercentage, flagType, setVariants to only test
conditionSet.rolloutPercentage !== undefined && flagType === "multivariate",
preserving the same ctx.addIssue call (with message "Multivariate condition sets
use variant rollout percentages" and path [...path, "rolloutPercentage"]).
In `@server/src/services/featureFlags/evaluator.ts`:
- Around line 139-145: The selection loop over variants can miss a match when
bucket === 100; update the condition inside the for (const variant of variants)
loop (which uses cumulativeRollout, clampPercentage, variant.rolloutPercentage
and bucket) so that a variant is returned when bucket < cumulativeRollout OR
when bucket === 100 && cumulativeRollout === 100 (i.e., treat the 100 boundary
as included), ensuring totalRollout still uses Math.min(100, cumulativeRollout).
- Around line 112-129: getConditionSets currently fabricates a default condition
set for flags that don't have persisted conditionSets, causing downstream
consumers to see a synthetic conditionSet and getConditionSetName to return
"condition_1"; change getConditionSets to only return flag.conditionSets when
Array.isArray(flag.conditionSets) && flag.conditionSets.length > 0 and otherwise
return an empty array (do not construct a synthetic set), and ensure
getConditionSetName is only relied on when a real condition set exists (i.e.
callers should not call getConditionSetName for empty results); update any
callers that currently assume a returned set exists to handle an empty array
instead.
🪄 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: f9d9331f-c76b-4fd1-91fa-0b7f38b013a1
📒 Files selected for processing (24)
client/messages/cs.jsonclient/messages/de.jsonclient/messages/en.jsonclient/messages/es.jsonclient/messages/fr.jsonclient/messages/it.jsonclient/messages/ja.jsonclient/messages/ko.jsonclient/messages/pl.jsonclient/messages/pt.jsonclient/messages/uk.jsonclient/messages/zh.jsonclient/src/api/analytics/endpoints/featureFlags.tsclient/src/api/analytics/endpoints/index.tsclient/src/app/[site]/feature-flags/page.tsxserver/drizzle/0006_first_logan.sqlserver/drizzle/meta/0006_snapshot.jsonserver/src/analytics-script/types.tsserver/src/api/featureFlags/index.tsserver/src/api/featureFlags/schemas.tsserver/src/db/postgres/schema.tsserver/src/index.tsserver/src/services/featureFlags/evaluator.test.tsserver/src/services/featureFlags/evaluator.ts
✅ Files skipped from review due to trivial changes (3)
- client/src/api/analytics/endpoints/index.ts
- server/drizzle/meta/0006_snapshot.json
- client/messages/zh.json
🚧 Files skipped from review as they are similar to previous changes (6)
- server/drizzle/0006_first_logan.sql
- server/src/index.ts
- client/src/api/analytics/endpoints/featureFlags.ts
- server/src/api/featureFlags/index.ts
- client/src/app/[site]/feature-flags/page.tsx
- server/src/analytics-script/types.ts
Comment on lines
+163
to
+166
| const hasConditionSetVariants = conditionSets.some(conditionSet => (conditionSet.variants?.length ?? 0) > 0); | ||
|
|
||
| if (!hasConditionSetVariants && variants.length < 2) { | ||
| ctx.addIssue({ |
Contributor
There was a problem hiding this comment.
Validate fallback top-level variants when some condition sets omit their own variants.
hasConditionSetVariants becomes true as soon as one condition set defines variants, but the evaluator still falls back to the flag-level variants for matching sets that don't. That lets a multivariate flag pass validation with a single top-level variant and then behave like a one-arm experiment for those audiences.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/src/api/featureFlags/schemas.ts` around lines 163 - 166, The current
check using hasConditionSetVariants lets a flag pass when only some
conditionSets define their own variants; change the validation so that if any
conditionSet defines variants (compute e.g. hasConditionSetWithVariants =
conditionSets.some(cs => (cs.variants?.length ?? 0) > 0)), then require the
top-level variants array to have at least two entries; update the conditional
that calls ctx.addIssue to use this new predicate (instead of the old
hasConditionSetVariants) and ensure the error message reflects that top-level
variants must be multivariate when any condition set relies on the fallback.
Comment on lines
+240
to
+246
| if (conditionSet.rolloutPercentage !== undefined && flagType === "multivariate" && setVariants.length > 0) { | ||
| ctx.addIssue({ | ||
| code: z.ZodIssueCode.custom, | ||
| message: "Multivariate condition sets use variant rollout percentages", | ||
| path: [...path, "rolloutPercentage"], | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
Reject rolloutPercentage on all multivariate condition sets.
The evaluator never reads conditionSet.rolloutPercentage in the multivariate path, including when a set inherits top-level variants. Right now configs like { rolloutPercentage: 10 } are accepted here but ignored at runtime.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/src/api/featureFlags/schemas.ts` around lines 240 - 246, The validator
currently only rejects conditionSet.rolloutPercentage for multivariate sets when
setVariants.length > 0; change the check so any conditionSet.rolloutPercentage
!== undefined triggers ctx.addIssue when flagType === "multivariate" (remove the
setVariants.length > 0 requirement). Update the if guard around the ctx.addIssue
call that references conditionSet.rolloutPercentage, flagType, setVariants to
only test conditionSet.rolloutPercentage !== undefined && flagType ===
"multivariate", preserving the same ctx.addIssue call (with message
"Multivariate condition sets use variant rollout percentages" and path [...path,
"rolloutPercentage"]).
Comment on lines
+112
to
+129
| function getConditionSets(flag: FeatureFlagRow): FeatureFlagConditionSet[] { | ||
| if (Array.isArray(flag.conditionSets) && flag.conditionSets.length > 0) { | ||
| return flag.conditionSets; | ||
| } | ||
|
|
||
| return [ | ||
| { | ||
| rules: Array.isArray(flag.rules) ? flag.rules : [], | ||
| rolloutPercentage: flag.rolloutPercentage, | ||
| variants: Array.isArray(flag.variants) ? flag.variants : [], | ||
| payload: flag.payload, | ||
| }, | ||
| ]; | ||
| } | ||
|
|
||
| function getConditionSetName(conditionSet: FeatureFlagConditionSet, index: number) { | ||
| return conditionSet.name || `condition_${index + 1}`; | ||
| } |
Contributor
There was a problem hiding this comment.
Don't expose a synthetic conditionSet for flags without configured sets.
getConditionSets() fabricates a default set and getConditionSetName() turns it into "condition_1". That means plain flags now report a conditionSet value even though no real set exists, which can leak fake segmentation into downstream consumers or exposure analytics. Only populate conditionSet when the flag actually has persisted condition sets.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/src/services/featureFlags/evaluator.ts` around lines 112 - 129,
getConditionSets currently fabricates a default condition set for flags that
don't have persisted conditionSets, causing downstream consumers to see a
synthetic conditionSet and getConditionSetName to return "condition_1"; change
getConditionSets to only return flag.conditionSets when
Array.isArray(flag.conditionSets) && flag.conditionSets.length > 0 and otherwise
return an empty array (do not construct a synthetic set), and ensure
getConditionSetName is only relied on when a real condition set exists (i.e.
callers should not call getConditionSetName for empty results); update any
callers that currently assume a returned set exists to handle an empty array
instead.
Comment on lines
+139
to
+145
| for (const variant of variants) { | ||
| cumulativeRollout += clampPercentage(variant.rolloutPercentage, 0); | ||
| if (bucket < cumulativeRollout) { | ||
| return { | ||
| variant, | ||
| totalRollout: Math.min(100, cumulativeRollout), | ||
| }; |
Contributor
There was a problem hiding this comment.
Handle the bucket === 100 boundary in variant selection.
bucketPercentage() can return exactly 100, but this branch only matches bucket < cumulativeRollout. On that boundary, a variant allocation totaling 100% still falls through instead of assigning a variant.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/src/services/featureFlags/evaluator.ts` around lines 139 - 145, The
selection loop over variants can miss a match when bucket === 100; update the
condition inside the for (const variant of variants) loop (which uses
cumulativeRollout, clampPercentage, variant.rolloutPercentage and bucket) so
that a variant is returned when bucket < cumulativeRollout OR when bucket ===
100 && cumulativeRollout === 100 (i.e., treat the 100 boundary as included),
ensuring totalRollout still uses Math.min(100, cumulativeRollout).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/src/app/[site]/feature-flags/page.tsx (1)
16-17:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSwitch this page to next-intl
useTranslations()for localized user-facing text (incl. the title).
client/src/app/[site]/feature-flags/page.tsxlines 16-17 currently useuseExtracted()and hardcode"Feature Flags"; for production UI i18n, replaceuseExtracted()withuseTranslations()and get the page title from a message key (instead of an English literal).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/src/app/`[site]/feature-flags/page.tsx around lines 16 - 17, Replace the non-i18n hook useExtracted() with next-intl's useTranslations() and fetch the page title from the locale messages instead of the hardcoded string; specifically, import and call useTranslations (e.g., const t = useTranslations('FeatureFlags') or appropriate namespace), then pass t('title') (or the agreed message key, e.g., 'featureFlags.title') into useSetPageTitle instead of "Feature Flags"; also update any other user-facing literals in this file to use t(...) so the page is fully localized.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@client/src/app/`[site]/feature-flags/components/ConditionSetsEditor.tsx:
- Around line 11-27: The component imports and calls useExtracted (in
ConditionSetsEditor) which is incorrect for next-intl; replace the import and
hook call with useTranslations from next-intl and ensure the variable usage
stays correct (e.g., const t = useTranslations(); then keep using t("...") for
conditionSetLabel), updating the import line that currently references
useExtracted and any types if necessary so translation strings resolve at build
time.
In `@client/src/app/`[site]/feature-flags/components/FeatureFlagTable.tsx:
- Line 136: The container div in FeatureFlagTable.tsx uses light-first Tailwind
classes (border-neutral-100 bg-white) with dark: overrides; change to dark-first
by making the default classes the dark-theme ones (e.g., border-neutral-850
bg-neutral-900) and add light: variants for the light theme (e.g.,
light:border-neutral-100 light:bg-white) so the className on the div reflects
dark-first styling.
In `@client/src/app/`[site]/feature-flags/lib/form.ts:
- Around line 77-247: The code currently performs manual runtime validation
inside parseOptionalPayload, parseRequiredPayload, parseRuleValue and
buildPayload (buildRules, buildVariants, validateVariants) — move these checks
into Zod schemas and call schema.parse at submit time instead of manual
branching: create Zod schemas for FeatureFlagPayload, FeatureFlagRule,
VariantFormState, ConditionSetFormState and FlagFormState that capture required
fields, types, array element constraints, rollout bounds, and unique-keys
constraint (use refinements for uniqueness and sum checks), replace
parseRuleValue/parseOptionalPayload/parseRequiredPayload logic with schema
coercions/parsers, and update buildPayload to simply transform the
already-validated parsed object into the API shape (or call schema.parse within
buildPayload once) rather than performing manual validation inside
buildRules/buildVariants/validateVariants.
- Around line 249-251: The buildPayload function currently returns key:
form.key.trim() even when that trimmed string is empty; modify buildPayload to
trim the key first, validate that the trimmed value is not an empty string, and
fail-fast (e.g., throw a clear Error or return a rejected result) if it is empty
so the dialog flow can surface the validation immediately; update the code
around the existing key: form.key.trim() return (reference buildPayload and
form.key) to perform this check before constructing and returning the payload.
---
Outside diff comments:
In `@client/src/app/`[site]/feature-flags/page.tsx:
- Around line 16-17: Replace the non-i18n hook useExtracted() with next-intl's
useTranslations() and fetch the page title from the locale messages instead of
the hardcoded string; specifically, import and call useTranslations (e.g., const
t = useTranslations('FeatureFlags') or appropriate namespace), then pass
t('title') (or the agreed message key, e.g., 'featureFlags.title') into
useSetPageTitle instead of "Feature Flags"; also update any other user-facing
literals in this file to use t(...) so the page is fully localized.
🪄 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: 1b37fa16-7d49-4a8b-b771-79834716653c
📒 Files selected for processing (23)
client/messages/cs.jsonclient/messages/de.jsonclient/messages/en.jsonclient/messages/es.jsonclient/messages/fr.jsonclient/messages/it.jsonclient/messages/ja.jsonclient/messages/ko.jsonclient/messages/pl.jsonclient/messages/pt.jsonclient/messages/uk.jsonclient/messages/zh.jsonclient/src/app/[site]/feature-flags/components/ConditionSetsEditor.tsxclient/src/app/[site]/feature-flags/components/FeatureFlagDialog.tsxclient/src/app/[site]/feature-flags/components/FeatureFlagSkeleton.tsxclient/src/app/[site]/feature-flags/components/FeatureFlagTable.tsxclient/src/app/[site]/feature-flags/components/TargetingRulesEditor.tsxclient/src/app/[site]/feature-flags/components/VariantsEditor.tsxclient/src/app/[site]/feature-flags/lib/constants.tsclient/src/app/[site]/feature-flags/lib/form.tsclient/src/app/[site]/feature-flags/lib/labels.tsclient/src/app/[site]/feature-flags/lib/types.tsclient/src/app/[site]/feature-flags/page.tsx
💤 Files with no reviewable changes (11)
- client/messages/pt.json
- client/messages/ko.json
- client/messages/de.json
- client/messages/es.json
- client/messages/pl.json
- client/messages/en.json
- client/messages/it.json
- client/messages/uk.json
- client/messages/ja.json
- client/messages/fr.json
- client/messages/cs.json
✅ Files skipped from review due to trivial changes (3)
- client/src/app/[site]/feature-flags/components/FeatureFlagSkeleton.tsx
- client/src/app/[site]/feature-flags/lib/labels.ts
- client/messages/zh.json
Comment on lines
+11
to
+27
| import { useExtracted } from "next-intl"; | ||
| import { createEmptyConditionSet } from "../lib/form"; | ||
| import type { ConditionSetFormState } from "../lib/types"; | ||
| import { TargetingRulesEditor } from "./TargetingRulesEditor"; | ||
| import { VariantsEditor } from "./VariantsEditor"; | ||
|
|
||
| export function ConditionSetsEditor({ | ||
| flagType, | ||
| conditionSets, | ||
| onChange, | ||
| }: { | ||
| flagType: FeatureFlagType; | ||
| conditionSets: ConditionSetFormState[]; | ||
| onChange: (conditionSets: ConditionSetFormState[]) => void; | ||
| }) { | ||
| const t = useExtracted(); | ||
| const conditionSetLabel = flagType === "remote_config" ? t("Targeted configs") : t("Release conditions"); |
Contributor
There was a problem hiding this comment.
Replace useExtracted with useTranslations for i18n hooks.
next-intl usage at Line 11/Line 26 appears incorrect and likely breaks typing/build; in this path we also require useTranslations().
In next-intl v4.8.3, does a hook named `useExtracted` exist? What is the correct hook for translating UI strings in client components?
As per coding guidelines, "Use next-intl's 'useTranslations()' hook for i18n; maintain translation files in messages/ directory (en.json, de.json, etc.)".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@client/src/app/`[site]/feature-flags/components/ConditionSetsEditor.tsx
around lines 11 - 27, The component imports and calls useExtracted (in
ConditionSetsEditor) which is incorrect for next-intl; replace the import and
hook call with useTranslations from next-intl and ensure the variable usage
stays correct (e.g., const t = useTranslations(); then keep using t("...") for
conditionSetLabel), updating the import line that currently references
useExtracted and any types if necessary so translation strings resolve at build
time.
| }; | ||
|
|
||
| return ( | ||
| <div className="rounded-lg border border-neutral-100 bg-white dark:border-neutral-850 dark:bg-neutral-900"> |
Contributor
There was a problem hiding this comment.
Use dark-first classes for default theme.
Line 136 sets a light default (bg-white) and only overrides in dark mode; this should be inverted so dark is default.
As per coding guidelines, "Dark mode is default theme".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@client/src/app/`[site]/feature-flags/components/FeatureFlagTable.tsx at line
136, The container div in FeatureFlagTable.tsx uses light-first Tailwind classes
(border-neutral-100 bg-white) with dark: overrides; change to dark-first by
making the default classes the dark-theme ones (e.g., border-neutral-850
bg-neutral-900) and add light: variants for the light theme (e.g.,
light:border-neutral-100 light:bg-white) so the className on the div reflects
dark-first styling.
Comment on lines
+77
to
+247
| function parseOptionalPayload(value: string): FeatureFlagPayloadValue | undefined { | ||
| const trimmed = value.trim(); | ||
| if (!trimmed) return undefined; | ||
| return JSON.parse(trimmed) as FeatureFlagPayloadValue; | ||
| } | ||
|
|
||
| function parseRequiredPayload(value: string): FeatureFlagPayloadValue { | ||
| const parsed = parseOptionalPayload(value); | ||
| if (parsed === undefined) { | ||
| throw new Error("Payload is required"); | ||
| } | ||
| return parsed; | ||
| } | ||
|
|
||
| function formatRuleValue(value: FeatureFlagRule["value"]) { | ||
| if (Array.isArray(value)) return JSON.stringify(value); | ||
| return String(value); | ||
| } | ||
|
|
||
| function parseRuleValue(value: string): FeatureFlagRule["value"] { | ||
| const trimmed = value.trim(); | ||
| if (trimmed.startsWith("[")) { | ||
| const parsed = JSON.parse(trimmed); | ||
| if ( | ||
| Array.isArray(parsed) && | ||
| parsed.every(item => typeof item === "string" || typeof item === "number" || typeof item === "boolean") | ||
| ) { | ||
| return parsed; | ||
| } | ||
| throw new Error("Rule arrays can only contain strings, numbers, or booleans"); | ||
| } | ||
| return trimmed; | ||
| } | ||
|
|
||
| function toRuleFormState(rule: FeatureFlagRule): RuleFormState { | ||
| return { | ||
| id: createRuleId(), | ||
| field: rule.field, | ||
| key: rule.key || "", | ||
| operator: rule.operator, | ||
| value: formatRuleValue(rule.value), | ||
| }; | ||
| } | ||
|
|
||
| function toVariantFormState(variant: FeatureFlag["variants"][number]): VariantFormState { | ||
| return { | ||
| id: createRuleId(), | ||
| key: variant.key, | ||
| name: variant.name || "", | ||
| rolloutPercentage: variant.rolloutPercentage, | ||
| payload: formatPayloadValue(variant.payload), | ||
| }; | ||
| } | ||
|
|
||
| function toConditionSetFormState( | ||
| conditionSet: FeatureFlag["conditionSets"][number], | ||
| flagType: FeatureFlagType, | ||
| index: number | ||
| ): ConditionSetFormState { | ||
| return { | ||
| id: createRuleId(), | ||
| name: conditionSet.name || (index === 0 ? "Default" : `Condition ${index + 1}`), | ||
| rules: (conditionSet.rules || []).map(toRuleFormState), | ||
| rolloutPercentage: conditionSet.rolloutPercentage ?? 100, | ||
| variants: | ||
| conditionSet.variants && conditionSet.variants.length > 0 | ||
| ? conditionSet.variants.map(toVariantFormState) | ||
| : flagType === "multivariate" | ||
| ? [createEmptyVariant(0), createEmptyVariant(1)] | ||
| : [], | ||
| payload: formatPayloadValue(conditionSet.payload), | ||
| }; | ||
| } | ||
|
|
||
| function fallbackConditionSetFromFlag(flag: FeatureFlag): ConditionSetFormState { | ||
| return { | ||
| id: createRuleId(), | ||
| name: "Default", | ||
| rules: (flag.rules || []).map(toRuleFormState), | ||
| rolloutPercentage: flag.rolloutPercentage, | ||
| variants: flag.flagType === "multivariate" ? (flag.variants || []).map(toVariantFormState) : [], | ||
| payload: formatPayloadValue(flag.payload), | ||
| }; | ||
| } | ||
|
|
||
| export function toFormState(flag?: FeatureFlag): FlagFormState { | ||
| if (!flag) return createEmptyForm(); | ||
|
|
||
| return { | ||
| key: flag.key, | ||
| description: flag.description || "", | ||
| enabled: flag.enabled, | ||
| runtime: flag.runtime, | ||
| flagType: flag.flagType, | ||
| conditionSets: | ||
| flag.conditionSets && flag.conditionSets.length > 0 | ||
| ? flag.conditionSets.map((conditionSet, index) => toConditionSetFormState(conditionSet, flag.flagType, index)) | ||
| : [fallbackConditionSetFromFlag(flag)], | ||
| }; | ||
| } | ||
|
|
||
| export function buildPayload(form: FlagFormState): FeatureFlagPayload { | ||
| const buildRules = (rules: RuleFormState[]) => | ||
| rules.map(rule => { | ||
| const requiresKey = rule.field === "query" || rule.field === "trait"; | ||
| if (requiresKey && !rule.key.trim()) { | ||
| throw new Error("Rule key is required"); | ||
| } | ||
| if (!rule.value.trim()) { | ||
| throw new Error("Rule value is required"); | ||
| } | ||
|
|
||
| return { | ||
| field: rule.field, | ||
| key: requiresKey ? rule.key.trim() : undefined, | ||
| operator: rule.operator, | ||
| value: parseRuleValue(rule.value), | ||
| }; | ||
| }); | ||
|
|
||
| const buildVariants = (variants: VariantFormState[]) => | ||
| variants.map(variant => ({ | ||
| key: variant.key.trim(), | ||
| name: variant.name.trim() || undefined, | ||
| rolloutPercentage: variant.rolloutPercentage, | ||
| payload: parseOptionalPayload(variant.payload), | ||
| })); | ||
|
|
||
| const validateVariants = (variants: ReturnType<typeof buildVariants>) => { | ||
| const variantKeys = new Set(variants.map(variant => variant.key)); | ||
| const variantRolloutTotal = variants.reduce((sum, variant) => sum + variant.rolloutPercentage, 0); | ||
|
|
||
| if (variants.length < 2) { | ||
| throw new Error("Multiple variant flags need at least two variants"); | ||
| } | ||
| if (variants.some(variant => !variant.key)) { | ||
| throw new Error("Variant key is required"); | ||
| } | ||
| if (variantKeys.size !== variants.length) { | ||
| throw new Error("Variant keys must be unique"); | ||
| } | ||
| if (variantRolloutTotal > 100) { | ||
| throw new Error("Variant rollout percentages cannot exceed 100"); | ||
| } | ||
| }; | ||
|
|
||
| const conditionSets = form.conditionSets.map(conditionSet => { | ||
| if ( | ||
| !Number.isInteger(conditionSet.rolloutPercentage) || | ||
| conditionSet.rolloutPercentage < 0 || | ||
| conditionSet.rolloutPercentage > 100 | ||
| ) { | ||
| throw new Error("Rollout must be between 0 and 100"); | ||
| } | ||
|
|
||
| const variants = form.flagType === "multivariate" ? buildVariants(conditionSet.variants) : []; | ||
| if (form.flagType === "multivariate") { | ||
| validateVariants(variants); | ||
| } | ||
|
|
||
| return { | ||
| name: conditionSet.name.trim() || undefined, | ||
| rules: buildRules(conditionSet.rules), | ||
| rolloutPercentage: form.flagType === "boolean" ? conditionSet.rolloutPercentage : undefined, | ||
| variants: form.flagType === "multivariate" ? variants : undefined, | ||
| payload: | ||
| form.flagType === "remote_config" | ||
| ? parseRequiredPayload(conditionSet.payload) | ||
| : parseOptionalPayload(conditionSet.payload), | ||
| }; | ||
| }); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Move runtime validation to Zod schemas instead of manual parsing branches.
This file currently hand-rolls validation/parsing logic for rules, payloads, and variants. In this client path, runtime validation should be expressed with Zod schemas and parsed once at submit time.
As per coding guidelines, "Use TypeScript strict mode throughout; use Zod for runtime validation".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@client/src/app/`[site]/feature-flags/lib/form.ts around lines 77 - 247, The
code currently performs manual runtime validation inside parseOptionalPayload,
parseRequiredPayload, parseRuleValue and buildPayload (buildRules,
buildVariants, validateVariants) — move these checks into Zod schemas and call
schema.parse at submit time instead of manual branching: create Zod schemas for
FeatureFlagPayload, FeatureFlagRule, VariantFormState, ConditionSetFormState and
FlagFormState that capture required fields, types, array element constraints,
rollout bounds, and unique-keys constraint (use refinements for uniqueness and
sum checks), replace parseRuleValue/parseOptionalPayload/parseRequiredPayload
logic with schema coercions/parsers, and update buildPayload to simply transform
the already-validated parsed object into the API shape (or call schema.parse
within buildPayload once) rather than performing manual validation inside
buildRules/buildVariants/validateVariants.
Comment on lines
+249
to
+251
| return { | ||
| key: form.key.trim(), | ||
| description: form.description.trim() || null, |
Contributor
There was a problem hiding this comment.
Reject empty feature-flag keys before returning payload.
key: form.key.trim() at Line 250 allows an empty key through. Add a required-key check in buildPayload to fail fast in the dialog flow.
Suggested fix
export function buildPayload(form: FlagFormState): FeatureFlagPayload {
+ const normalizedKey = form.key.trim();
+ if (!normalizedKey) {
+ throw new Error("Key is required");
+ }
+
const buildRules = (rules: RuleFormState[]) =>
@@
return {
- key: form.key.trim(),
+ key: normalizedKey,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return { | |
| key: form.key.trim(), | |
| description: form.description.trim() || null, | |
| export function buildPayload(form: FlagFormState): FeatureFlagPayload { | |
| const normalizedKey = form.key.trim(); | |
| if (!normalizedKey) { | |
| throw new Error("Key is required"); | |
| } | |
| const buildRules = (rules: RuleFormState[]) => | |
| // ... rest of function | |
| return { | |
| key: normalizedKey, | |
| description: form.description.trim() || null, | |
| // ... rest of return object |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@client/src/app/`[site]/feature-flags/lib/form.ts around lines 249 - 251, The
buildPayload function currently returns key: form.key.trim() even when that
trimmed string is empty; modify buildPayload to trim the key first, validate
that the trimmed value is not an empty string, and fail-fast (e.g., throw a
clear Error or return a rejected result) if it is empty so the dialog flow can
surface the validation immediately; update the code around the existing key:
form.key.trim() return (reference buildPayload and form.key) to perform this
check before constructing and returning the payload.
This commit updates language files for multiple languages, adding new keys and translations to improve user experience. Additionally, it enhances the UI of the feature flag editor by introducing helper text for condition sets, improving layout consistency, and adding icons for better visual representation. These changes aim to provide clearer instructions and a more intuitive interface for managing feature flags.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
client/messages/ja.json (1)
78-78:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPopulate the new Japanese entries before merge.
Changed keys are still empty (for example, Line [78], Line [249], Line [416]). That will surface blank text in Japanese UI.
As per coding guidelines,
client/messages/**/*.json: "Keep locale keys consistent acrossmessages/*.json."Also applies to: 249-250, 259-262, 266-274, 276-276, 281-282, 297-298, 301-302, 306-306, 308-308, 416-416
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/messages/ja.json` at line 78, The Japanese locale file client/messages/ja.json contains empty translation values (e.g., key "mZDrWI" and other keys around lines referenced) which will render blank text in the UI; populate each empty value with the correct Japanese translation matching the corresponding keys in other locale files (keep keys identical to source keys used in messages/*.json), ensuring all changed keys listed (including the groups around lines 249-250, 259-262, 266-274, 276, 281-282, 297-298, 301-302, 306, 308, and 416) are filled with appropriate Japanese strings and follow the existing formatting/escaping conventions.client/messages/fr.json (1)
78-78:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not leave newly introduced French keys empty.
New keys are added with empty values (e.g., Line [78], Line [249], Line [416]). That creates blank UI copy in affected feature flows.
As per coding guidelines,
client/messages/**/*.json: "Keep locale keys consistent acrossmessages/*.json."Also applies to: 249-250, 259-262, 266-274, 276-276, 281-282, 297-298, 301-302, 306-306, 308-308, 416-416
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/messages/fr.json` at line 78, The French locale file has newly introduced keys left empty (e.g., "mZDrWI" and the other keys at the locations noted); update client/messages/fr.json by adding proper French translations for each new key to match the corresponding entries in the default locale (or source messages file) so UI copy is not blank—locate the untranslated keys such as "mZDrWI" and the other empty keys referenced in the comment and provide accurate French strings consistent with existing message keys and formatting conventions.client/messages/pl.json (1)
78-78:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFill newly added Polish strings instead of shipping empty values.
Several newly added keys are
""(for example, Line [78], Line [249], Line [416]). These will render blank labels/messages in UI paths that use them.As per coding guidelines,
client/messages/**/*.json: "Keep locale keys consistent acrossmessages/*.json."Also applies to: 249-250, 259-262, 266-274, 276-276, 281-282, 297-298, 301-302, 306-306, 308-308, 416-416
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/messages/pl.json` at line 78, Replace all newly added Polish locale entries that are empty (for example the key "mZDrWI" and any other keys currently set to "") with their proper Polish translations, mirroring the intent and phrasing from the corresponding keys in other locale files so locale keys remain consistent; update each empty string value to a meaningful Polish string matching the source/EN text and keep the JSON structure intact while preserving the existing keys.client/messages/es.json (1)
78-78:⚠️ Potential issue | 🟠 Major | ⚡ Quick winComplete missing Spanish translations.
Approximately 23 translation keys have empty string values, which will render as blank labels in the Spanish UI. This breaks the user experience for Spanish-speaking users who won't see critical text for navigation, buttons, or messages.
Examples:
- Line 78:
"mZDrWI": ""- Lines 249-274: Multiple funnel/editor-related keys
- Lines 281-282, 297-298, 301-302, 306, 308: Additional empty values
Ensure all keys have proper Spanish translations before merging.
Also applies to: 249-250, 259-262, 266-274, 276-276, 281-282, 297-298, 301-302, 306-306, 308-308
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/messages/es.json` at line 78, Several Spanish translation keys currently have empty values (e.g., "mZDrWI" and the funnel/editor-related keys around the 249–274, 281–282, 297–302, 306, 308 ranges), resulting in blank UI labels; replace each empty string with an accurate Spanish translation preserving any interpolation/placeholders/ICU tokens exactly as in the source keys (do not change key names like "mZDrWI"), ensure the JSON remains valid (quoting/commas intact), and run the i18n/lint/tests to verify no missing translations remain before merging.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@client/messages/cs.json`:
- Around line 249-250: Several Czech locale entries are empty (e.g., keys
"U5hCuK", "wmdo1a" and the other keys noted), which will render blank UI text;
populate each of these keys in client/messages/cs.json with a proper Czech
translation (or temporarily copy the English value from the source
locale/messages file as a clear fallback) so no string value is "" and locale
key sets remain consistent across messages/*.json; update all occurrences the
reviewer flagged (lines with keys around 249, 259–262, 266–274, 276, 281–282,
297–298, 301–302, 306, 308) ensuring each key has a non-empty value.
In `@client/messages/uk.json`:
- Around line 249-250: Several message keys (e.g., "U5hCuK", "wmdo1a" and other
empty keys listed: ranges around keys at positions 259-262, 266-274, 276,
281-282, 297-298, 301-302, 306, 308) in the Ukrainian locale are empty; replace
each empty string with the correct Ukrainian translation matching the intent
from the canonical locale (usually en.json) and ensure every key present in
other locales exists and is non-empty here; update the corresponding entries in
client/messages/uk.json by providing Ukrainian text for each empty key and keep
key names exactly as in other locales so the messages/*.json key set stays
consistent across locales.
---
Outside diff comments:
In `@client/messages/es.json`:
- Line 78: Several Spanish translation keys currently have empty values (e.g.,
"mZDrWI" and the funnel/editor-related keys around the 249–274, 281–282,
297–302, 306, 308 ranges), resulting in blank UI labels; replace each empty
string with an accurate Spanish translation preserving any
interpolation/placeholders/ICU tokens exactly as in the source keys (do not
change key names like "mZDrWI"), ensure the JSON remains valid (quoting/commas
intact), and run the i18n/lint/tests to verify no missing translations remain
before merging.
In `@client/messages/fr.json`:
- Line 78: The French locale file has newly introduced keys left empty (e.g.,
"mZDrWI" and the other keys at the locations noted); update
client/messages/fr.json by adding proper French translations for each new key to
match the corresponding entries in the default locale (or source messages file)
so UI copy is not blank—locate the untranslated keys such as "mZDrWI" and the
other empty keys referenced in the comment and provide accurate French strings
consistent with existing message keys and formatting conventions.
In `@client/messages/ja.json`:
- Line 78: The Japanese locale file client/messages/ja.json contains empty
translation values (e.g., key "mZDrWI" and other keys around lines referenced)
which will render blank text in the UI; populate each empty value with the
correct Japanese translation matching the corresponding keys in other locale
files (keep keys identical to source keys used in messages/*.json), ensuring all
changed keys listed (including the groups around lines 249-250, 259-262,
266-274, 276, 281-282, 297-298, 301-302, 306, 308, and 416) are filled with
appropriate Japanese strings and follow the existing formatting/escaping
conventions.
In `@client/messages/pl.json`:
- Line 78: Replace all newly added Polish locale entries that are empty (for
example the key "mZDrWI" and any other keys currently set to "") with their
proper Polish translations, mirroring the intent and phrasing from the
corresponding keys in other locale files so locale keys remain consistent;
update each empty string value to a meaningful Polish string matching the
source/EN text and keep the JSON structure intact while preserving the existing
keys.
🪄 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: 1ba42928-e6f3-4dc1-ab15-d9020a80ef90
📒 Files selected for processing (16)
client/messages/cs.jsonclient/messages/de.jsonclient/messages/en.jsonclient/messages/es.jsonclient/messages/fr.jsonclient/messages/it.jsonclient/messages/ja.jsonclient/messages/ko.jsonclient/messages/pl.jsonclient/messages/pt.jsonclient/messages/uk.jsonclient/messages/zh.jsonclient/src/app/[site]/feature-flags/components/ConditionSetsEditor.tsxclient/src/app/[site]/feature-flags/components/FeatureFlagDialog.tsxclient/src/app/[site]/feature-flags/components/TargetingRulesEditor.tsxclient/src/app/[site]/feature-flags/components/VariantsEditor.tsx
🚧 Files skipped from review as they are similar to previous changes (9)
- client/src/app/[site]/feature-flags/components/ConditionSetsEditor.tsx
- client/src/app/[site]/feature-flags/components/TargetingRulesEditor.tsx
- client/src/app/[site]/feature-flags/components/VariantsEditor.tsx
- client/src/app/[site]/feature-flags/components/FeatureFlagDialog.tsx
- client/messages/en.json
- client/messages/de.json
- client/messages/it.json
- client/messages/pt.json
- client/messages/ko.json
Comment on lines
+249
to
+250
| "U5hCuK": "", | ||
| "wmdo1a": "", |
Contributor
There was a problem hiding this comment.
Avoid shipping empty Czech translations for newly added keys.
A large part of the newly added Czech key set is still "" (e.g., Line [249], Line [266], Line [301]). These map to visible controls and can render blank UI text.
As per coding guidelines, client/messages/**/*.json: "Keep locale keys consistent across messages/*.json."
Also applies to: 259-262, 266-274, 276-276, 281-282, 297-298, 301-302, 306-306, 308-308
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@client/messages/cs.json` around lines 249 - 250, Several Czech locale entries
are empty (e.g., keys "U5hCuK", "wmdo1a" and the other keys noted), which will
render blank UI text; populate each of these keys in client/messages/cs.json
with a proper Czech translation (or temporarily copy the English value from the
source locale/messages file as a clear fallback) so no string value is "" and
locale key sets remain consistent across messages/*.json; update all occurrences
the reviewer flagged (lines with keys around 249, 259–262, 266–274, 276,
281–282, 297–298, 301–302, 306, 308) ensuring each key has a non-empty value.
Comment on lines
+249
to
+250
| "U5hCuK": "", | ||
| "wmdo1a": "", |
Contributor
There was a problem hiding this comment.
Complete the new Ukrainian translations in this block.
Many changed keys are still empty strings (for example, Line [249], Line [266], Line [301]). In production this can yield blank UI text for Ukrainian users.
As per coding guidelines, client/messages/**/*.json: "Keep locale keys consistent across messages/*.json."
Also applies to: 259-262, 266-274, 276-276, 281-282, 297-298, 301-302, 306-306, 308-308
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@client/messages/uk.json` around lines 249 - 250, Several message keys (e.g.,
"U5hCuK", "wmdo1a" and other empty keys listed: ranges around keys at positions
259-262, 266-274, 276, 281-282, 297-298, 301-302, 306, 308) in the Ukrainian
locale are empty; replace each empty string with the correct Ukrainian
translation matching the intent from the canonical locale (usually en.json) and
ensure every key present in other locales exists and is non-empty here; update
the corresponding entries in client/messages/uk.json by providing Ukrainian text
for each empty key and keep key names exactly as in other locales so the
messages/*.json key set stays consistent across locales.
This commit introduces a new JSON payload editor in the feature flag management interface, allowing users to add and edit payloads more intuitively. It also enhances the UI by improving layout consistency and adding icons for better visual representation. Additionally, translations for multiple languages are updated to reflect these changes, ensuring a cohesive user experience across the application.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/src/app/[site]/feature-flags/components/FeatureFlagDialog.tsx (1)
14-40:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSwitch FeatureFlagDialog to use
useTranslations()(notuseExtracted())File:
client/src/app/[site]/feature-flags/components/FeatureFlagDialog.tsx
The dialog follows the project/i18n rule to use next-intl’suseTranslations()for UI strings.useExtractedis an experimental extraction API and requires special configuration, so it shouldn’t be used here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/src/app/`[site]/feature-flags/components/FeatureFlagDialog.tsx around lines 14 - 40, The component FeatureFlagDialog currently imports and calls useExtracted() to get translations; replace that with next-intl's stable useTranslations() by updating the import and the hook call (replace useExtracted import and the const t = useExtracted(); line with useTranslations and const t = useTranslations(/* optional namespace if used */)); ensure any downstream usage of t remains compatible. Locate FLAG_TYPE_ICONS, FeatureFlagDialog, and the t variable to make the swap consistently across the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@client/src/app/`[site]/feature-flags/components/FeatureFlagDialog.tsx:
- Around line 14-40: The component FeatureFlagDialog currently imports and calls
useExtracted() to get translations; replace that with next-intl's stable
useTranslations() by updating the import and the hook call (replace useExtracted
import and the const t = useExtracted(); line with useTranslations and const t =
useTranslations(/* optional namespace if used */)); ensure any downstream usage
of t remains compatible. Locate FLAG_TYPE_ICONS, FeatureFlagDialog, and the t
variable to make the swap consistently across the file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0a469b44-efdf-47fd-bbae-56ecc19bf579
⛔ Files ignored due to path filters (1)
client/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
client/messages/cs.jsonclient/messages/de.jsonclient/messages/en.jsonclient/messages/es.jsonclient/messages/fr.jsonclient/messages/it.jsonclient/messages/ja.jsonclient/messages/ko.jsonclient/messages/pl.jsonclient/messages/pt.jsonclient/messages/uk.jsonclient/messages/zh.jsonclient/package.jsonclient/src/app/[site]/feature-flags/components/ConditionSetsEditor.tsxclient/src/app/[site]/feature-flags/components/FeatureFlagDialog.tsxclient/src/app/[site]/feature-flags/components/FeatureFlagTable.tsxclient/src/app/[site]/feature-flags/components/JsonEditor.tsxclient/src/app/[site]/feature-flags/components/VariantsEditor.tsxclient/src/app/[site]/feature-flags/lib/form.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- client/src/app/[site]/feature-flags/components/VariantsEditor.tsx
- client/src/app/[site]/feature-flags/components/FeatureFlagTable.tsx
- client/messages/ko.json
- client/messages/ja.json
- client/messages/en.json
- client/messages/cs.json
- client/messages/pl.json
- client/messages/de.json
- client/src/app/[site]/feature-flags/lib/form.ts
- client/messages/it.json
- client/messages/zh.json
- client/messages/fr.json
This commit introduces comprehensive documentation for feature flags, including usage examples in the tracking script and API specifications. It also updates the tagging documentation to reference feature flags for runtime value selection. Additionally, the server-side implementation is enhanced with regex validation for feature flag rules and improved evaluation methods, ensuring robust handling of feature flags in analytics.
…re for specific endpoint
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
1 participant
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 by CodeRabbit
New Features
Tests
Chores