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.

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(insights): keep configuration when navigating between insights #17156

Merged
merged 30 commits into from
Nov 3, 2023

Conversation

thmsobrmlr
Copy link
Contributor

@thmsobrmlr thmsobrmlr commented Aug 23, 2023

Problem

Users want to switch from one query to another while keeping relevant configuration.

Changes

This PR re-adds said functionality.

(Potential) Follow-ups

  • An error is thrown when switching from trends -> funnels -> trends with the compare option enabled. The reason for this is that the results are still based on the funnel response for a brief moment. Once this is fixed, the compare option can be enabled here as well.
  • Event and action nodes include math properties which are only supported in some insights e.g. trends, but not in others e.g. funnels. Not all math properties supported in trends are supported in stickiness insights e.g. count per user average. Should we have different types for them and convert this accordingly? We seem to handle this well backend side, but frontend side is broken in some cases e.g. stickiness insight with unsupported math type.
    In the same spirit retentionFilter.target_entity and returning_entity should not have an order property.

How did you test this code?

Added tests and locally, with 👀

@PostHog PostHog deleted a comment from posthog-bot Aug 23, 2023
@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

thmsobrmlr and others added 2 commits September 12, 2023 12:06
# Conflicts:
#	playwright/e2e-vrt/scenes-app/insights.spec.ts-snapshots/annotations-popover-displays-correctly-1-chromium-linux.png
@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 2)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@thmsobrmlr thmsobrmlr marked this pull request as ready for review October 26, 2023 15:17
@PostHog PostHog deleted a comment from posthog-bot Oct 26, 2023
@PostHog PostHog deleted a comment from posthog-bot Oct 26, 2023
@PostHog PostHog deleted a comment from posthog-bot Oct 26, 2023
@PostHog PostHog deleted a comment from posthog-bot Oct 26, 2023
@PostHog PostHog deleted a comment from posthog-bot Oct 26, 2023
@PostHog PostHog deleted a comment from posthog-bot Oct 26, 2023
@PostHog PostHog deleted a comment from posthog-bot Oct 26, 2023
@thmsobrmlr thmsobrmlr removed the stale label Oct 26, 2023
@thmsobrmlr thmsobrmlr requested a review from a team October 26, 2023 15:48
Copy link
Member

@Twixes Twixes left a comment

Choose a reason for hiding this comment

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

I like the approach here. Just found one bug: The configuration is kept as expected when a saved insight is loaded and immediately switched to a different type – but it's not kept when I first load the saved insights page and get to that same insight from there.

@thmsobrmlr thmsobrmlr force-pushed the navigate-between-queries branch from 1f7575e to 14a558f Compare November 2, 2023 11:58
@thmsobrmlr
Copy link
Contributor Author

Awesome catches @Twixes, thanks for the thorough look!

Just found one bug: The configuration is kept as expected when a saved insight is loaded and immediately switched to a different type – but it's not kept when I first load the saved insights page and get to that same insight from there.

This is because the insightDataLogic isn't mounted yet when insightLogic calls setInsight and so the respective listener doesn't fire. I've added a query cache update in insightNavLogic's afterMount.

There'd be an alternative approach that removes the problematic code altogether, simplifying a bit what happens when loading an insight. I've been trying it out here #18346, but am a bit worried about unintended effects of this.

@thmsobrmlr thmsobrmlr force-pushed the navigate-between-queries branch from 1304ed0 to 6171a81 Compare November 2, 2023 12:23
@PostHog PostHog deleted a comment from posthog-bot Nov 2, 2023
@PostHog PostHog deleted a comment from posthog-bot Nov 2, 2023
@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

3 snapshot changes in total. 0 added, 3 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (wasn't pushed!)
  • webkit: 0 added, 2 modified, 0 deleted (wasn't pushed!)

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@thmsobrmlr thmsobrmlr merged commit 9f7931d into master Nov 3, 2023
73 checks passed
@thmsobrmlr thmsobrmlr deleted the navigate-between-queries branch November 3, 2023 09:14
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.

3 participants