feat(flags): warn when a fractional rollout breaks local evaluation - #85597
Conversation
A release condition rollout percentage that is not a whole number fails to parse on .NET before 2.13.3 and Java before 2.12.1, which stops local evaluation for every flag in the project rather than just the edited flag. Show a warning above the release conditions naming the offending percentages and the minimum SDK versions. Skip client-only flags, which never run local evaluation, and skip multivariate variant rollouts, which those SDKs have always read as floats. Generated-By: PostHog Desktop Task-Id: b46f1d31-74ad-4a17-854c-4019b1e4e020
github-actions
Bot
added
feature/feature-flags
🤖 CI report
|
| File | Size | Δ vs base |
|---|---|---|
render-query/src/render-query/render-query.js |
21.86 MiB | 🔺 +14.5 KiB (+0.1%) |
posthog-app/_parent/products/workflows/frontend/WorkflowsScene.js |
89.4 KiB | 🔺 +4.0 KiB (+4.7%) |
posthog-app/src/scenes/onboarding/Onboarding.js |
794.0 KiB | 🔺 +1.3 KiB (+0.2%) |
Posted automatically by build-bundle-size-report · uncompressed bytes from dist-report
✅ Eager graph — within budget
How much code each root ships on the eager path — downloaded and parsed before the surface is interactive. Measured from the esbuild output chunks (post-tree-shake, static imports only); lazy import() / React.lazy chunks are not counted.
| Root | Eager (shipped) | Δ vs base | Budget |
|---|---|---|---|
entry (logged-out pages, app bootstrap)src/index.tsx |
1.35 MiB · 22 files | 🔺 +237 B (+0.0%) | ███░░░░░░░ 30.0% of 4.51 MiB |
authenticated shell (every logged-in page)src/scenes/AuthenticatedShell.tsx |
8.59 MiB · 3,164 files | 🔺 +16.4 KiB (+0.2%) | █████████░ 88.5% of 9.71 MiB |
🟢 node_modules/monaco-editor/ stays out of src/index.tsx
🟢 src/lib/components/ActivityLog/describers stays out of src/index.tsx
🟢 [object Object] stays out of src/index.tsx
🟢 [object Object] stays out of src/index.tsx
🟢 node_modules/monaco-editor/ stays out of src/scenes/AuthenticatedShell.tsx
🟢 src/lib/components/ActivityLog/describers stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
Largest files eagerly shipped from src/index.tsx
| Size | File |
|---|---|
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 24.6 KiB | ../node_modules/.pnpm/buffer@6.0.3/node_modules/buffer/index.js |
| 6.3 KiB | ../node_modules/.pnpm/react@18.3.1/node_modules/react/cjs/react.production.min.js |
| 4.5 KiB | ../node_modules/.pnpm/@jspm+core@2.1.0/node_modules/@jspm/core/nodelibs/browser/process.js |
| 3.9 KiB | ../node_modules/.pnpm/scheduler@0.23.2/node_modules/scheduler/cjs/scheduler.production.min.js |
| 1.4 KiB | ../node_modules/.pnpm/base64-js@1.5.1/node_modules/base64-js/index.js |
| 1.3 KiB | src/RootErrorBoundary.tsx |
| 912 B | ../node_modules/.pnpm/ieee754@1.2.1/node_modules/ieee754/index.js |
| 789 B | src/scenes/ChunkLoadErrorBoundary.tsx |
| 762 B | src/index.tsx |
Largest files eagerly shipped from src/scenes/AuthenticatedShell.tsx
| Size | File |
|---|---|
| 304.7 KiB | ../node_modules/.pnpm/posthog-js@1.417.4/node_modules/posthog-js/dist/rrweb.js |
| 267.7 KiB | ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 246.9 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 243.9 KiB | ../node_modules/.pnpm/posthog-js@1.417.4/node_modules/posthog-js/dist/module.js |
| 154.2 KiB | ../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js |
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 104.2 KiB | src/lib/api.ts |
| 95.0 KiB | ../packages/quill/packages/quill/dist/index.js |
| 93.3 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
| 90.6 KiB | ../node_modules/.pnpm/@tiptap+core@3.20.6_@tiptap+pm@3.20.6/node_modules/@tiptap/core/dist/index.js |
Posted automatically by check-eager-graph · sizes are eager output bytes (shipped, post-tree-shake) from the esbuild metafile · part of #32479
✅ Toolbar bundle — eager 2.23 MiB within budget
What the toolbar ships to customer pages, measured from the esbuild output (minified, post-tree-shake). The eager set is the entry plus everything statically imported from it — fetched before any feature runs; deferred chunks load lazily. The eager guardrail is 5.72 MiB. Each output file must also stay below 10 MB, where CloudFront stops compressing it. The module boundary is enforced separately by check-toolbar-graph.
| Metric | Size | Δ vs base | Budget |
|---|---|---|---|
| Eager (shipped) entry + static imports |
2.23 MiB · 17 files | 🔺 +824 B (+0.0%) | ████░░░░░░ 39.0% of 5.72 MiB |
| Deferred (lazy) | 2.09 MiB · 33 files | no change | n/a — loads on demand |
Loader dist/toolbar.js |
1.1 KiB | no change | █░░░░░░░░░ 5.8% of 19.5 KiB |
Largest eagerly-shipped chunks
| Size | File |
|---|---|
| 743.8 KiB | dist/toolbar/toolbar-app-6ZKNXNH5.css |
| 566.2 KiB | dist/toolbar/chunk-chunk-JL4FKUSE.js |
| 484.6 KiB | dist/toolbar/chunk-chunk-KGJBZAGK.js |
| 133.6 KiB | dist/toolbar/chunk-chunk-SR5BOEHA.js |
| 131.8 KiB | dist/toolbar/chunk-chunk-T5KY5WYR.js |
| 71.2 KiB | dist/toolbar/toolbar-app-CI2HZHSD.js |
| 69.0 KiB | dist/toolbar/chunk-chunk-27JL52RE.js |
| 35.6 KiB | dist/toolbar/chunk-chunk-GWXGBVE4.js |
| 20.9 KiB | dist/toolbar/chunk-chunk-PGHVZTNN.js |
| 12.2 KiB | dist/toolbar/chunk-chunk-PIK3PADE.js |
Posted automatically by check-toolbar-size · sizes are toolbar output bytes (shipped, post-tree-shake) from the esbuild metafile
✅ Dist folder size — 🔺 +341.9 KiB (+0.0%)
Total size of the built frontend/dist folder (all assets), compared against the base branch.
Total: 1454.87 MiB · 🔺 +341.9 KiB (+0.0%)
⚠️ Playwright — 1 flaky
🎭 Playwright report · View test results →
- Can delete a person (chromium)
These issues are not necessarily caused by your changes.
Annoyed by this section? Help fix flakies and failures and it will go green!
|
Screenshot: 
|
🦔 Hogbox preview · ✅ ready▶ Open the preview
commit |
Prompt To Fix All With AI### Issue 1
products/feature_flags/frontend/FractionalRolloutWarning.test.tsx:28
**Consolidate the test suite structure**
This test file introduces two top-level `describe` blocks and an `it.each` callback without an explicit return type, diverging from the repository's required Jest organization and frontend TypeScript conventions and adding avoidable inconsistency for future changes.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "feat(flags): warn when a fractional roll..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
🦔 ReviewHog reviewed this pull requestNothing worth raising this time, so here's a calming picture instead:
Resolved comments: 1 already settled |
…untime The local evaluation payload carries client-only flags too; SDKs filter by runtime only after parsing the whole document, so a client-only flag with a fractional rollout still breaks .NET < 2.13.3 and Java < 2.12.1. Drop the runtime gate and the prop that fed it.
AGENTS.md asks for a single top-level describe block per Jest file. Group the helper and rendering cases as nested describes instead of two top-level ones. Test bodies and coverage are unchanged. Generated-By: PostHog Desktop Task-Id: b46f1d31-74ad-4a17-854c-4019b1e4e020
haacked
left a comment
haacked
left a comment
There was a problem hiding this comment.
Nice warning! A few suggestions inline — one test gap, a UX question about the readonly surfaces, and some small copy/display nits.
| SDKs read rollout percentages as whole numbers before .NET {MIN_DOTNET_VERSION} and Java {MIN_JAVA_VERSION}. | ||
| On an older version the flag definitions payload fails to parse, so local evaluation stops working for{' '} | ||
| <strong>every flag in the project</strong> and each evaluation goes to the <code>/flags</code> endpoint | ||
| instead. Upgrade those SDKs, or use a whole number here.{' '} |
There was a problem hiding this comment.
question: The same warning renders on the readonly surfaces (FeatureFlagReleaseConditionsReadonly.tsx:155 and the collapsible at :1124), but its copy ends with "Upgrade those SDKs, or use a whole number here." On a readonly surface — an experiment viewing a flag's conditions — the viewer often can't edit the flag, so "use a whole number here" points at a control that isn't present. Should the readonly contexts drop the "use a whole number here" clause?
There was a problem hiding this comment.
Good catch. Rather than vary the copy per surface, the clause now reads "change the rollout to a whole number" instead of "use a whole number here". Dropping the "here" removes the pointer at a control that is not on screen, while keeping the advice actionable for a viewer who can go and edit the flag or pass it on. That avoids threading a readonly prop through three call sites for a wording difference.
|
|
||
| return ( | ||
| <LemonBanner type="warning" className={className}> | ||
| This flag has a fractional rollout percentage ({percentages.join('%, ')}%). The .NET and Java server-side |
There was a problem hiding this comment.
nit: The banner opens with the singular "This flag has a fractional rollout percentage" even when it lists several values like "(0.5%, 33.33%)" from the test at FractionalRolloutWarning.test.tsx:149. The plural would read better in the multi-value case.
|
|
||
| <FeatureFlagConditionWarning properties={properties} evaluationRuntime={evaluationRuntime} /> | ||
|
|
||
| <FractionalRolloutWarning filterGroups={filterGroups} /> |
There was a problem hiding this comment.
question: The editor surface passes className="mb-3" at FeatureFlagReleaseConditions.tsx:588, but the collapsible (FeatureFlagReleaseConditionsCollapsible.tsx:1124) and readonly (FeatureFlagReleaseConditionsReadonly.tsx) surfaces render <FractionalRolloutWarning filterGroups={filterGroups} /> with no spacing class. On those two surfaces the banner sits flush against the next element, unlike the sibling LemonBanners around it which use mb-3. Should the collapsible and readonly instances also pass className="mb-3" to match?
There was a problem hiding this comment.
Checked the three surfaces, and the asymmetry is load-bearing rather than an oversight.
The editor renders inside SceneSection, which is not a flex container, so its banners carry their own margins (mb-3 on the disabled banner at FeatureFlagReleaseConditions.tsx:583, mt-3 mb-3 on the variant-override one at :593). mb-3 is right there.
The collapsible and readonly surfaces both render inside <div className="flex flex-col gap-2"> (FeatureFlagReleaseConditionsCollapsible.tsx:1085, FeatureFlagReleaseConditionsReadonly.tsx:137), so the parent already spaces children. Their immediate sibling FeatureFlagConditionWarning passes no class on either surface, and IntentWarningsBanner passes none in the collapsible. Adding mb-3 there would stack a margin on top of gap-2 and make this banner the odd one out next to the warning directly above it.
Left as is. The mb-3 on the disabled banner inside the collapsible flex container is arguably the actual outlier, but that predates this PR.
| import { FractionalRolloutWarning, fractionalRolloutPercentages } from './FractionalRolloutWarning' | ||
|
|
||
| function group(rollout_percentage: number | null): FeatureFlagGroupType { | ||
| return { properties: [], rollout_percentage, variant: null, sort_key: `group-${rollout_percentage}` } |
There was a problem hiding this comment.
suggestion: The one behavior the PR singles out as intentional — that a condition group's variant assignment never triggers the warning — has no test. The group() factory here always sets variant: null, so no case exercises a group that carries a variant name. Per the data model, such a group with a fractional rollout_percentage should still warn (the group rollout is what breaks the parse), and a group with a whole-number rollout should stay silent regardless of variant.
Without a case pinning this, a future change that starts keying off group.variant could suppress a real warning or fire a spurious one, and the suite stays green. Extend the factory to accept a variant and add a case mirroring stays silent when every rollout percentage is a whole number right below it.
| return { properties: [], rollout_percentage, variant: null, sort_key: `group-${rollout_percentage}` } | |
| function group(rollout_percentage: number | null, variant: string | null = null): FeatureFlagGroupType { | |
| return { properties: [], rollout_percentage, variant, sort_key: `group-${rollout_percentage}-${variant}` } | |
| } |
Then assert renderWarning([group(0.5, 'test')]) still warns and renderWarning([group(50, 'test')]) stays silent.
There was a problem hiding this comment.
Added, with one clarification worth flagging because the two senses of "variant" differ.
The exclusion the PR description calls out is multivariate variant rollouts (filters.multivariate.variants[].rollout_percentage), not a condition group's variant override string. The component only ever reads group.rollout_percentage, and its signature takes filterGroups rather than the whole filters object, so it structurally cannot see the multivariate block. That exclusion is enforced by the type, not by a runtime branch, so there is nothing at this level for a unit test to pin.
Your underlying point still holds for group.variant, and that one is real and untested: a group with a variant override and a fractional rollout does occur in practice, and a future change that keyed off group.variant could suppress it. Took the factory suggestion and added a parameterized case asserting group(0.5, 'test') warns and group(50, 'test') stays silent.
|
|
||
| return ( | ||
| <LemonBanner type="warning" className={className}> | ||
| This flag has a fractional rollout percentage ({percentages.join('%, ')}%). The .NET and Java server-side |
There was a problem hiding this comment.
suggestion: FractionalRolloutWarning.tsx:39 builds the percentage list with percentages.join('%, ') plus a trailing %, which renders a raw JavaScript float. A rollout stored as something like 33.333333 (or a float-arithmetic artifact) would print all its digits inside the banner, e.g. "(33.333333%)", which reads as a bug to the user rather than a clear warning.
Round for display to keep the message legible without changing the detection logic. Consider formatting each value before joining, for example: ({percentages.map((p) => ${+p.toFixed(2)}%).join(', ')})
There was a problem hiding this comment.
Applied, with one deviation from the suggested formatter that matters.
+p.toFixed(2) collapses a genuine sub-0.01 rollout to 0, so a flag set to 0.00015% would render "(0%)" — a warning that misstates the value it is warning about, which is worse than printing too many digits. Rollouts below 0.01 are reachable through the API even though the editor input floors at two decimal places.
Used significant digits instead:
function formatPercentage(percentage: number): string {
return `${Number(percentage.toPrecision(4))}%`
}That gives 33.333333333333336 -> "33.33%", 49.2537313433 -> "49.25%", and 0.00015 -> "0.00015%". Both cases are pinned in the test file so the toFixed trap does not get reintroduced.
Three copy and display fixes from review: The banner said "This flag has a fractional rollout percentage" even when it listed several values, so the multi-value case now reads "percentages". Percentages were rendered straight from the stored float. A flag saved through the API can hold more precision than the editor's two decimal places, so a value like 33.333333333333336 printed in full and read as a bug. Trim to four significant digits rather than two fixed decimals, so a sub-0.01 rollout such as 0.00015 still shows its real value instead of rounding to 0. The closing sentence said "use a whole number here", which points at an input that does not exist on the readonly and collapsible surfaces. It now says "change the rollout to a whole number", which reads correctly everywhere. Also pin two behaviors that had no coverage: a group carrying a variant override still warns on a fractional rollout and stays silent on a whole one, and high-precision values are trimmed without collapsing to zero. Generated-By: PostHog Desktop Task-Id: b46f1d31-74ad-4a17-854c-4019b1e4e020
2 participants
Problem
Someone editing a feature flag can set a release condition rollout to a fractional percentage such as 0.5%, and nothing in the UI says older server-side SDKs cannot read it. On .NET before 2.13.3 and Java before 2.12.1 the flag definitions payload fails to parse, so local evaluation stops working for every flag in the project and each evaluation goes to
/flagsinstead. The edited flag looks fine, so the breakage surfaces as unexplained request volume from a service nobody was touching.Variant splits are not affected. Those SDKs typed only the condition group field as an integer, so a fractional variant split, which an even three-way split cannot avoid, has always parsed. A warning that fired on variants would be noise.
Follows the write-path fix in #84957 and the backfill in #85042. Those stop whole numbers being widened to
100.0; they cannot help a flag whose rollout is genuinely fractional, which is what this warns about.Changes
products/feature_flags/frontend/, rendered from the three existing release-condition surfaces (editor, collapsible, and readonly).@testing-library/reactjoins the product's devDependencies so a component test can run there, matching howproducts/ai_observabilityis set up.No screenshot captured: this environment had no browser available, so the rendering was not checked visually. The warning is a standard
LemonBanner type="warning", placed where the existing local-evaluation condition warning already sits.How did you test this code?
Automated only. No manual browser check was run.
FractionalRolloutWarning.test.tsxcovers the regressions no existing suite caught: a fractional group rollout warns and names every offending value, several fractional condition sets are all listed, and a whole-number rollout stays silent.Automatic notifications
Docs update
No docs page currently states which SDK versions read a fractional rollout percentage, so the warning links to the general local evaluation page. Worth a short note on that page as a follow-up.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Written by the PostHog Slack app (Claude Opus 5) from the Slack thread linked below, where a flag owner asked for this warning after the wire-format change was traced. Assignee is deliberately unset: the person who directed the work was identified only by their Slack display name, and guessing a GitHub handle from that risks tagging an unrelated account. The requester should self-assign as DRI.
Repo skills consulted while producing this:
/writing-user-facing-copy(which caught em-dashes in the first draft of the banner copy, and a "not just this one" antithesis),/placing-product-frontend-code(itsscene_product_split.pyscript reported feature flags as mid-migration and sent the new files toproducts/feature_flags/frontend/rather thanfrontend/src/scenes/feature-flags/),/writing-pr-descriptions, and the frontend guide's reuse rule, which is why this is aLemonBannerrather than a hand-rolled callout.Two decisions worth a reviewer's attention. The warning was first written as an extra entry in the existing
featureFlagConditionWarningLogicissue list; that was wrong, because that component's copy promises the flag still works normally through the API, whereas this failure disables local evaluation project-wide. It became a sibling component instead. Scoping it to condition groups rather than all rollout percentages came from reading the two SDK fixes:posthog-dotnet#292 andposthog-android#699 each widened exactly one field, and the variant field was already a float in both.Nothing in this PR carries material from the originating session that is not already public: no customer names, ticket contents, quoted thread text, or usage figures. The example percentages in the copy and tests are invented.
Created with PostHog from a Slack thread