Skip to content

fix(retention): mark custom bracket cells as in-progress when window hasn't elapsed#58689

Open
okxint wants to merge 9 commits into
PostHog:masterfrom
okxint:fix/retention-bracket-in-progress
Open

fix(retention): mark custom bracket cells as in-progress when window hasn't elapsed#58689
okxint wants to merge 9 commits into
PostHog:masterfrom
okxint:fix/retention-bracket-in-progress

Conversation

@okxint
Copy link
Copy Markdown
Contributor

@okxint okxint commented May 17, 2026

Problem

Custom retention bracket columns show 0% instead of graying out with "Period in progress" for cohorts whose bracket window hasn't fully closed yet. For example, with brackets [1, 3, 4, 8] weeks and a cohort starting Feb 1 2026, the "weeks 9–16" column shows 0% on May 17 even though the window doesn't close until May 24.

Individual period columns correctly detect in-progress state by comparing the cell date against today. The same logic was never applied to custom bracket columns.

Fixes #58679

Changes

In retentionLogic.ts, the isCurrentPeriod calculation is updated to handle custom brackets:

  • Individual periods (unchanged): cellDate.isSame(now, periodUnit) — marks the current period column
  • Custom brackets (new): A bracket cell is in-progress when its END period offset (cumulative sum of all bracket sizes up to that index) is still after today — i.e. cohort_date + bracketEndOffset > now

Example with brackets [1, 3, 4, 8] weeks, cohort Feb 1 2026, today May 17 2026:

  • Index 4 → bracketEndOffset = 1+3+4+8 = 16 → end date = Feb 1 + 16 weeks = May 24 → isAfter(now)in-progress

How did you test this code?

Automated: TypeScript compilation passes. The logic change is isolated to the isCurrentPeriod computation inside retentionLogic.ts. Manual testing requires a retention insight with custom brackets and a cohort whose latest bracket window straddles today.

Publish to changelog?

No

🤖 Agent context

Authored with Claude Code. Root cause traced to retentionLogic.ts — the per-value isCurrentPeriod check used cellDate.isSame(now, periodUnit) uniformly, but for custom brackets cellDate is the bracket's start, not end. The fix accumulates bracket sizes to find the bracket's true end date and compares that against now.

okxint added 9 commits May 13, 2026 23:22
When a user clicks the "Average time to convert" value in a funnel, the
viz switches to the histogram view with no way to return to the steps
view. Inject a forceBackTo breadcrumb pointing to the saved insight URL
whenever funnelVizType is TimeToConvert, so the back button appears and
navigates cleanly back to the steps view.

Fixes PostHog#57540
The duplicate action used a non-null assertion on hogFunction.template
despite a TODO comment acknowledging template could be absent. Functions
created without a template would throw at runtime when accessing
template.id. Add an early return with a user-facing error toast so the
failure is visible rather than a silent crash.
…ined

The supportPlans selector chained optional access on addons followed by
a direct .flat() call. If addons resolves to undefined at runtime the
chain returns undefined, making .flat() throw and ...addonPlans in splice
throw "not iterable". Tighten to platformAndSupportProduct.addons ?? []
so the fallback is always an iterable array.
The deleteSurveySuccess listener used String(action.payload) to read
the deleted survey's ID. kea-loaders wraps the original input and the
return value together in the success action payload, so action.payload
was the wrapper object, producing "[object Object]" instead of the ID.

Destructure { payload: surveyId } from the first listener argument
(which IS action.payload) to correctly access the original deleteSurvey
input, and unskip the test that was skipped while the bug was open.
The cohort picker excluded not_in from both the operator dropdown and
from recent items, but the backend (flag_matching.py and
user_blast_radius.py) fully supports cohort not_in. Remove the exclusion
so users can target "users NOT in cohort X" from the release condition
builder without workarounds.
…guration

Port 465 uses implicit SSL (SMTPS) and requires EMAIL_USE_SSL=true.
EMAIL_USE_TLS enables STARTTLS, which is for port 587. When
EMAIL_PORT=465 and EMAIL_USE_TLS=true, Django's SMTP backend tries to
upgrade a plaintext connection that the server never offers, hanging
the connection indefinitely — which blocks the login request that
triggered the email send.

Two changes:
1. Pass EMAIL_TIMEOUT (default 10 s) to the SMTP backend constructor
   so misconfigured connections time out instead of hanging forever.
2. Log a warning when EMAIL_PORT=465 + EMAIL_USE_TLS=true is detected,
   explaining the correct setting (EMAIL_USE_SSL=true, EMAIL_USE_TLS=false).

EMAIL_TIMEOUT is added to DYNAMIC_SETTINGS so it can be overridden
via instance settings without a redeploy.

Fixes PostHog#57936
…oundary

Lazy-loaded components inside scenes (WorldMap, RegionMap,
TrendsCalendarHeatMap, BoxPlotChart, TrendsLineChart, TrendsBarChart
in Trends.tsx and NavTabChat in Nav.tsx) were wrapped only in Suspense,
not ChunkLoadErrorBoundary. On a stale-deploy chunk-hash mismatch the
error escaped the scene boundary and surfaced as an error card instead
of triggering the reload-once recovery already in place at app level.

Closes PostHog#55535
…to-hiding platforms

Base UI's ScrollArea.Viewport sets `overflow: scroll` as an inline style,
which renders a native scrollbar permanently even when content fits within
the container. PostHog's per-axis override was passing `undefined` for the
scrollable axis, letting Base UI's value win. Change it to `'auto'` so the
scrollbar only appears when content actually overflows.

Affected surfaces: all LemonTable instances, LemonCalendar time pickers,
popovers, side panels, navigation, and any other ScrollableShadows consumer.

Fixes PostHog#55041
…hasn't elapsed

Custom retention bracket columns were showing 0% instead of graying out
with "Period in progress" for cohorts whose bracket window hasn't fully
closed yet. Individual period columns correctly used isCurrentPeriod to
detect this, but the same logic was never applied to brackets.

For custom brackets, a cell is in-progress when the bracket's END period
offset (cumulative sum of bracket sizes) is still in the future, not just
its start date. Updated the isCurrentPeriod calculation in retentionLogic
to account for this by accumulating bracket sizes and comparing the end
date against now.

Fixes PostHog#58679
@assign-reviewers-posthog assign-reviewers-posthog Bot requested review from a team May 17, 2026 07:51
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 17, 2026

Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
frontend/src/scenes/feature-flags/cohortPickerProps.ts:20-23
The constant name `COHORTS_ONLY_SUPPORT_IN_PICKER_PROPS` now contradicts its implementation — the `not_in` operator is no longer excluded, so the "ONLY_SUPPORT_IN" part of the name is false. The comment was correctly updated to explain the change, but the exported name itself is used at three call sites and will mislead future readers into thinking the preset still restricts to `in`-only.

```suggestion
export const COHORTS_PICKER_PROPS = {
    excludedOperators: { [TaxonomicFilterGroupType.Cohorts]: [] as PropertyOperator[] },
    selectingKeyOnly: { [TaxonomicFilterGroupType.Cohorts]: true },
}
```

### Issue 2 of 2
frontend/src/scenes/retention/retentionLogic.ts:198-205
**Multiple brackets marked in-progress simultaneously**

For custom brackets, `bracketEndDate.isAfter(now)` will be `true` for every bracket whose end period is still in the future — not just the bracket currently straddling today. For example, with brackets `[1, 3, 4, 8]` and a cohort from 2 weeks ago, both the third and fourth brackets would have `isCurrentPeriod = true`. Individual periods only ever mark one cell in-progress. This may be intentional (all open windows should grey out), but worth confirming the UI handles multiple `isCurrentPeriod = true` values in the same row gracefully.

Reviews (1): Last reviewed commit: "fix(retention): mark custom bracket cell..." | Re-trigger Greptile

Comment on lines 20 to 23
export const COHORTS_ONLY_SUPPORT_IN_PICKER_PROPS = {
excludedOperators: { [TaxonomicFilterGroupType.Cohorts]: [PropertyOperator.NotIn] },
excludedOperators: { [TaxonomicFilterGroupType.Cohorts]: [] as PropertyOperator[] },
selectingKeyOnly: { [TaxonomicFilterGroupType.Cohorts]: true },
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 The constant name COHORTS_ONLY_SUPPORT_IN_PICKER_PROPS now contradicts its implementation — the not_in operator is no longer excluded, so the "ONLY_SUPPORT_IN" part of the name is false. The comment was correctly updated to explain the change, but the exported name itself is used at three call sites and will mislead future readers into thinking the preset still restricts to in-only.

Suggested change
export const COHORTS_ONLY_SUPPORT_IN_PICKER_PROPS = {
excludedOperators: { [TaxonomicFilterGroupType.Cohorts]: [PropertyOperator.NotIn] },
excludedOperators: { [TaxonomicFilterGroupType.Cohorts]: [] as PropertyOperator[] },
selectingKeyOnly: { [TaxonomicFilterGroupType.Cohorts]: true },
}
export const COHORTS_PICKER_PROPS = {
excludedOperators: { [TaxonomicFilterGroupType.Cohorts]: [] as PropertyOperator[] },
selectingKeyOnly: { [TaxonomicFilterGroupType.Cohorts]: true },
}
Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/scenes/feature-flags/cohortPickerProps.ts
Line: 20-23

Comment:
The constant name `COHORTS_ONLY_SUPPORT_IN_PICKER_PROPS` now contradicts its implementation — the `not_in` operator is no longer excluded, so the "ONLY_SUPPORT_IN" part of the name is false. The comment was correctly updated to explain the change, but the exported name itself is used at three call sites and will mislead future readers into thinking the preset still restricts to `in`-only.

```suggestion
export const COHORTS_PICKER_PROPS = {
    excludedOperators: { [TaxonomicFilterGroupType.Cohorts]: [] as PropertyOperator[] },
    selectingKeyOnly: { [TaxonomicFilterGroupType.Cohorts]: true },
}
```

How can I resolve this? If you propose a fix, please make it concise.

Comment on lines +198 to +205
if (customBrackets && customBrackets.length > 0 && index > 0) {
// Sum bracket sizes up to this index to get the end period offset.
// e.g. brackets [1,3,4,8] at index 4 → offset 16 → ends at cohort+16 periods
const bracketEndOffset = customBrackets
.slice(0, index)
.reduce((acc: number, size: number) => acc + size, 0)
const bracketEndDate = dayjs(result.date).tz(timezone).add(bracketEndOffset, periodUnit)
isCurrentPeriod = bracketEndDate.isAfter(now)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Multiple brackets marked in-progress simultaneously

For custom brackets, bracketEndDate.isAfter(now) will be true for every bracket whose end period is still in the future — not just the bracket currently straddling today. For example, with brackets [1, 3, 4, 8] and a cohort from 2 weeks ago, both the third and fourth brackets would have isCurrentPeriod = true. Individual periods only ever mark one cell in-progress. This may be intentional (all open windows should grey out), but worth confirming the UI handles multiple isCurrentPeriod = true values in the same row gracefully.

Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/scenes/retention/retentionLogic.ts
Line: 198-205

Comment:
**Multiple brackets marked in-progress simultaneously**

For custom brackets, `bracketEndDate.isAfter(now)` will be `true` for every bracket whose end period is still in the future — not just the bracket currently straddling today. For example, with brackets `[1, 3, 4, 8]` and a cohort from 2 weeks ago, both the third and fourth brackets would have `isCurrentPeriod = true`. Individual periods only ever mark one cell in-progress. This may be intentional (all open windows should grey out), but worth confirming the UI handles multiple `isCurrentPeriod = true` values in the same row gracefully.

How can I resolve this? If you propose a fix, please make it concise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Custom retention bracket cells show 0% instead of in-progress before bracket window closes

1 participant