Skip to content

fix(insights): Retain the legend configuration inside the subscriptions as well #41910

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Nov 21, 2025
Merged

fix(insights): Retain the legend configuration inside the subscriptions as well #41910

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f0f469f
chore: Extract the showLegend display option from the DB so that it a…
vdekrijger Nov 21, 2025
4032dd2
chore: Add the DB logic to fetch the show legend flag
vdekrijger Nov 21, 2025
c74314b
test: Add tests to cover the new path
vdekrijger Nov 21, 2025
79ae5bc
chore: Also force trigger a build of the workers on merge
vdekrijger Nov 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions frontend/src/queries/query.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ import { OnlineExportContext, QueryExportContext } from '~/types'

import {
HogQLQueryString,
getShowLegend,
isAsyncResponse,
isDataTableNode,
isDataVisualizationNode,
Expand All @@ -40,12 +39,8 @@ export function queryExportContext<N extends DataNode>(
if (isDataTableNode(query) || isDataVisualizationNode(query)) {
return queryExportContext(query.source, methodOptions, refresh)
} else if (isInsightQueryNode(query)) {
let showLegend = getShowLegend(query)

return {
source: query,
// Include show_legend for PNG exports
show_legend: showLegend === true,
}
} else if (isPersonsNode(query)) {
return { path: getPersonsEndpoint(query) }
Expand Down
1 change: 0 additions & 1 deletion frontend/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4487,7 +4487,6 @@ export type OnlineExportContext = {
export type QueryExportContext = {
source: Record<string, any>
filename?: string
show_legend?: boolean
}

export interface ReplayExportContext {
Expand Down
16 changes: 16 additions & 0 deletions posthog/models/insight.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,22 @@ def get_effective_query(
def url(self):
return absolute_uri(f"/insights/{self.short_id}")

@property
def show_legend(self) -> bool:
"""Extract show_legend display setting from insight configuration"""
try:
if self.query:
source = self.query.get("source", {})

# Check any filter which might contain `showLegend`
for value in source.values():
if isinstance(value, dict) and "showLegend" in value:
return bool(value.get("showLegend", False))
Comment on lines +300 to +302
Copy link
Contributor

@greptile-apps greptile-apps bot Nov 21, 2025

Choose a reason for hiding this comment

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

logic: iterating through all source.values() and returning the first dict with showLegend is fragile - dict iteration order could cause it to pick up showLegend from an unexpected location if the schema changes

Suggested change
for value in source.values():
if isinstance(value, dict) and "showLegend" in value:
return bool(value.get("showLegend", False))
# Check specific filter fields that contain showLegend
for filter_field in ['trendsFilter', 'lifecycleFilter', 'stickinessFilter']:
if filter_field in source and isinstance(source[filter_field], dict):
if 'showLegend' in source[filter_field]:
return bool(source[filter_field].get('showLegend', False))
Prompt To Fix With AI 
This is a comment left during a code review.
Path: posthog/models/insight.py
Line: 300:302

Comment:
**logic:** iterating through all `source.values()` and returning the first dict with `showLegend` is fragile - dict iteration order could cause it to pick up `showLegend` from an unexpected location if the schema changes

```suggestion
                # Check specific filter fields that contain showLegend
                for filter_field in ['trendsFilter', 'lifecycleFilter', 'stickinessFilter']:
                    if filter_field in source and isinstance(source[filter_field], dict):
                        if 'showLegend' in source[filter_field]:
                            return bool(source[filter_field].get('showLegend', False))
```

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

Copy link
Contributor Author

@vdekrijger vdekrijger Nov 21, 2025

Choose a reason for hiding this comment

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

I prefer my implementation as the proposed one is quite brittle when we are adding new filters / have these hidden string dependencies.

In our current structure it is very unlikely that there are multiple showLegend flags, and if there will be, it will return the first one it encounters meaning that it won't break anything.

Copy link
Contributor

@greptile-apps greptile-apps bot Nov 21, 2025

Choose a reason for hiding this comment

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

Got it! Your current implementation is indeed more flexible and maintainable. I've noted your preference to avoid hardcoded filter field names when extracting configuration values from dynamic structures.

Here's a link to the new rule that will be applied in future reviews of similar patterns.


return bool(self.filters.get("show_legend", False) if self.filters else False)
except (AttributeError, TypeError, KeyError):
return False

def generate_query_metadata(self):
from posthog.hogql_queries.query_metadata import extract_query_metadata

Expand Down
42 changes: 42 additions & 0 deletions posthog/models/test/test_insight_model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

from django.db.utils import IntegrityError

from parameterized import parameterized

from posthog.models import Dashboard, Insight, Team
from posthog.models.insight import generate_insight_filters_hash

Expand Down Expand Up @@ -225,3 +227,43 @@ def test_dashboard_with_query_insight_and_filters(self) -> None:
assert data
actual = data["source"]["filters"]
assert expected_filters == actual

@parameterized.expand(
[
(
"query_with_show_legend_true",
{
"query": {
"kind": "InsightVizNode",
"source": {
"kind": "TrendsQuery",
"trendsFilter": {"showLegend": True},
},
},
"filters": None,
"expected": True,
},
),
(
"legacy_filters_with_show_legend_true",
{
"query": None,
"filters": {"show_legend": True},
"expected": True,
},
),
(
"defaults_to_false",
{
"query": None,
"filters": None,
"expected": False,
},
),
]
)
def test_show_legend_property(self, name: str, test_case: dict) -> None:
insight = Insight.objects.create(
team=self.team, query=test_case["query"], filters=test_case.get("filters") or {}
)
assert insight.show_legend is test_case["expected"]
5 changes: 1 addition & 4 deletions posthog/tasks/exports/image_exporter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,10 +105,7 @@ def _export_to_png(exported_asset: ExportedAsset, max_height_pixels: Optional[in
wait_for_css_selector: CSSSelector
screenshot_height: int = 600
if exported_asset.insight is not None:
show_legend = False
if exported_asset.export_context:
show_legend = exported_asset.export_context.get("show_legend", False)

show_legend = exported_asset.insight.show_legend
legend_param = "&legend=true" if show_legend else ""
url_to_render = absolute_uri(f"/exporter?token={access_token}{legend_param}")
wait_for_css_selector = ".ExportedInsight"
Expand Down
2 changes: 1 addition & 1 deletion posthog/temporal/subscriptions/subscription_scheduling_workflow.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@

LOGGER = get_logger(__name__)

# Changed 21/11/2025
# Changed 21-Nov-2025


@dataclasses.dataclass
Expand Down
Loading