Skip to content
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

feat(insight-variables): dashboard variables populating and updating urls #27843

Merged
merged 13 commits into from
Jan 30, 2025
Merged

feat(insight-variables): dashboard variables populating and updating urls #27843

Changes from 3 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
5e222f8
dashboard variables populating and updating urls
EDsCODE Jan 23, 2025
661ccac
Merge branch 'master' into insight-variables-dashboard-urls
EDsCODE Jan 23, 2025
96f385a
Merge branch 'master' into insight-variables-dashboard-urls
EDsCODE Jan 23, 2025
3f60e0c
collapse URL vars to a json string
EDsCODE Jan 24, 2025
69e0042
Merge branch 'insight-variables-dashboard-urls' of github.com:PostHog…
EDsCODE Jan 24, 2025
0fff57b
Update UI snapshots for `chromium` (1)
github-actions[bot] Jan 24, 2025
cd96286
Merge branch 'master' into insight-variables-dashboard-urls
EDsCODE Jan 24, 2025
36b1837
merge main
EDsCODE Jan 24, 2025
4adff64
Merge branch 'master' into insight-variables-dashboard-urls
EDsCODE Jan 24, 2025
d6c0698
Merge branch 'master' into insight-variables-dashboard-urls
EDsCODE Jan 27, 2025
28f2e5b
clean up logic
EDsCODE Jan 29, 2025
f523830
Merge branch 'insight-variables-dashboard-urls' of github.com:PostHog…
EDsCODE Jan 29, 2025
c5cc8bd
Merge branch 'master' into insight-variables-dashboard-urls
EDsCODE Jan 29, 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
63 changes: 61 additions & 2 deletions frontend/src/scenes/dashboard/dashboardLogic.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@
sharedListeners,
} from 'kea'
import { loaders } from 'kea-loaders'
import { router, urlToAction } from 'kea-router'
import { actionToUrl, router, urlToAction } from 'kea-router'
import { subscriptions } from 'kea-subscriptions'
import api, { ApiMethodOptions, getJSONOrNull } from 'lib/api'
import { accessLevelSatisfied } from 'lib/components/AccessControlAction'
import { DashboardPrivilegeLevel, FEATURE_FLAGS, OrganizationMembershipLevel } from 'lib/constants'
Expand Down Expand Up @@ -261,6 +262,7 @@
}),
resetVariables: () => ({ variables: values.insightVariables }),
setAccessDeniedToDashboard: true,
setURLVariables: (variables: Record<string, Partial<HogQLVariable>>) => ({ variables }),
})),

loaders(({ actions, props, values }) => ({
Expand Down Expand Up @@ -495,6 +497,15 @@
: state,
},
],
maybeUrlVariables: [
EDsCODE marked this conversation as resolved.
Show resolved Hide resolved
{} as Record<string, Partial<HogQLVariable>>,
{
setURLVariables: (state, { variables }) => ({
...state,
...variables,
}),
},
],
insightVariables: [
{} as Record<string, HogQLVariable>,
{
Expand Down Expand Up @@ -1479,6 +1490,51 @@
},
})),

subscriptions(({ values, actions }) => ({
variables: (variables) => {
Copy link
Member

Choose a reason for hiding this comment

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

Should this not be maybeUrlVariables? Or is this triggered from somewhere else?

Copy link
Member Author

Choose a reason for hiding this comment

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

this is sort of dicey basically maybeURLVariables needs to be set first on load and then when variables loads, it'll do a check the maybe variables to populate if possible (which is why this subscription is mounted on variables)

// try to convert url variables to variables
const urlVariables = values.maybeUrlVariables
const overrideVariables = []

for (const [key, value] of Object.entries(urlVariables)) {
const variable = variables.find((variable: HogQLVariable) => variable.code_name === key)
if (variable) {
overrideVariables.push({ ...variable, value })
EDsCODE marked this conversation as resolved.
Show resolved Hide resolved
}
}
Copy link
Member

Choose a reason for hiding this comment

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

So I'm not a fan of allowing users to set any query param and have our logic try to parse it as variables. In the future, we can't use dashboard query params for anything else in case of a conflict of insight variable names. Instead, I think we should have a single query param (something like query_variables) that is a key/value pair object instead - this makes it a lot more explicit.

The downside to this is that it's more effort on the end of building the URL when writing a HogQL query, but we can add the overrides as a parameter of the <a variableOverrides={...} /> component instead (long term, links are done via chart settings instead of in the query itself)


overrideVariables.forEach((variable) => {
actions.overrideVariableValue(variable.id, variable.value)
})
},
})),

actionToUrl(({ values }) => ({
overrideVariableValue: ({ variableId, value, allVariables }) => {
const { currentLocation } = router.values

const currentVariable = allVariables.find((variable: HogQLVariable) => variable.id === variableId)

Check failure on line 1516 in frontend/src/scenes/dashboard/dashboardLogic.tsx

View workflow job for this annotation

GitHub Actions / Code quality checks 

Property 'id' does not exist on type 'HogQLVariable'.

if (!currentVariable) {
return [currentLocation.pathname, currentLocation.searchParams, currentLocation.hashParams]
}

const newUrlVariables = {
...values.maybeUrlVariables,
[currentVariable.code_name]: value,
}

return [
currentLocation.pathname,
{
...currentLocation.searchParams,
...newUrlVariables,
},
currentLocation.hashParams,
]
},
})),

urlToAction(({ values, actions }) => ({
'/dashboard/:id/subscriptions(/:subscriptionId)': ({ subscriptionId }) => {
const id = subscriptionId
Expand All @@ -1491,7 +1547,10 @@
actions.setDashboardMode(null, null)
},

'/dashboard/:id': () => {
'/dashboard/:id': (_, searchParams) => {
if (values.featureFlags[FEATURE_FLAGS.INSIGHT_VARIABLES]) {
actions.setURLVariables(searchParams)
}
actions.setSubscriptionMode(false, undefined)
actions.setTextTileId(null)
if (values.dashboardMode === DashboardMode.Sharing) {
Expand Down
Loading