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

chore: remove experiment creation from experiment scenes #28081

Merged
merged 1 commit into from
Jan 30, 2025

Conversation

joshsny
Copy link
Contributor

@joshsny joshsny commented Jan 30, 2025

Problem

We prompt users to create experiments from funnels, but this doesn't make sense if they are already viewing experiment results.

Changes

Hide "create experiment" button based on the currently active scene

Does this work well for both Cloud and self-hosted?

Yes

How did you test this code?

Manually

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

Modified the supportsCreatingExperiment selector in insightLogic.tsx to prevent experiment creation within experiment-related scenes, improving UX by avoiding recursive experiment creation.

  • Added scene-based validation in supportsCreatingExperiment to disable experiment creation in Scene.Experiment, Scene.Experiments, and experiment metrics scenes
  • Maintains existing query validation through isValidQueryForExperiment check
  • Prevents confusing UX where users could create experiments while already viewing experiment results

1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile

@@ -70,6 +72,8 @@ export const insightLogic: LogicWrapper<insightLogicType> = kea<insightLogicType
['user'],
featureFlagLogic,
['featureFlags'],
sceneLogic,
['activeScene'],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm pretty unsure about doing this - we should be fine as the logic is already mounted at the app level but can someone think if this is a bad idea?

Alternatively, we could pass a prop in the insightProps to avoid this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I don't know really. With a prop, the dependency is more explicit. But can't think of any strong arguments against this. Any similar logic in the code base we should be consistent with? Couldn't find any myself.

Copy link
Contributor Author

@joshsny joshsny Jan 30, 2025

Choose a reason for hiding this comment

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

Not really - as I mentioned there is InsightLogicProps but that's more for general properties of the insight. Tests are passing so I'm okay merging this if you can't think of a strong reason not to, and then reverting if it causes a problem, didn't find any issues when running locally.

@joshsny joshsny requested a review from a team January 30, 2025 11:57
Copy link
Contributor

Size Change: -10 B (0%)

Total Size: 1.16 MB

ℹ️ View Unchanged
Filename Size Change
frontend/dist/toolbar.js 1.16 MB -10 B (0%)

compressed-size-action

@joshsny joshsny requested a review from andehen January 30, 2025 14:28
@joshsny joshsny merged commit b71f538 into master Jan 30, 2025
102 checks passed
@joshsny joshsny deleted the remove-experiment-metric-inception branch January 30, 2025 14:57
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.

2 participants