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(experiments): Remove blocking results load animation #28066

Merged
merged 4 commits into from
Jan 30, 2025

Conversation

danielbachhuber
Copy link
Contributor

@danielbachhuber danielbachhuber commented Jan 29, 2025

See https://posthog.slack.com/archives/C07PXH2GTGV/p1738179059221599

When adding a secondary result, all results have to recalculate and it takes 2 minutes for each and so much waiting. This seems super unnecessary.

Changes

  • Removes <ExperimentLoadingAnimation /> from displaying when experiment results are loading.
  • Prevents <ExperimentImplementationDetails experiment={experiment} /> from displaying until results have loaded, to prevent it from displaying and then disappearing.
  • Introduces refreshExperimentResults() so we can null out the stale results while the new results are loading.

I didn't think it would be this simple, but seems to work in my testing.

Before

CleanShot 2025-01-29 at 15 21 31

After

CleanShot 2025-01-29 at 15 20 32

How did you test this code?

Manual testing. To more easily evaluate, apply this diff locally:

diff --git a/frontend/src/scenes/experiments/experimentLogic.tsx b/frontend/src/scenes/experiments/experimentLogic.tsx
index 8daf9df7de..84de49bc81 100644
--- a/frontend/src/scenes/experiments/experimentLogic.tsx
+++ b/frontend/src/scenes/experiments/experimentLogic.tsx
@@ -1051,7 +1051,8 @@ export const experimentLogic = kea<experimentLogicType>([
             null as (CachedExperimentTrendsQueryResponse | CachedExperimentFunnelsQueryResponse | null)[] | null,
             {
                 loadMetricResults: async (
-                    refresh?: boolean
+                    refresh?: boolean,
+                    breakpoint?: BreakPointFunction
                 ): Promise<(CachedExperimentTrendsQueryResponse | CachedExperimentFunnelsQueryResponse | null)[]> => {
                     let metrics = values.experiment?.metrics
                     const sharedMetrics = values.experiment?.saved_metrics
@@ -1070,6 +1071,8 @@ export const experimentLogic = kea<experimentLogicType>([
                                 }
                                 const response = await performQuery(queryWithExperimentId, undefined, refresh)
 
+                                await breakpoint(5000)
+
                                 return {
                                     ...response,
                                     fakeInsightId: Math.random().toString(36).substring(2, 15),
@@ -1103,7 +1106,8 @@ export const experimentLogic = kea<experimentLogicType>([
             null as (CachedExperimentTrendsQueryResponse | CachedExperimentFunnelsQueryResponse | null)[] | null,
             {
                 loadSecondaryMetricResults: async (
-                    refresh?: boolean
+                    refresh?: boolean,
+                    breakpoint?: BreakPointFunction
                 ): Promise<(CachedExperimentTrendsQueryResponse | CachedExperimentFunnelsQueryResponse | null)[]> => {
                     let metrics = values.experiment?.metrics_secondary
                     const sharedMetrics = values.experiment?.saved_metrics
@@ -1122,6 +1126,8 @@ export const experimentLogic = kea<experimentLogicType>([
                                 }
                                 const response = await performQuery(queryWithExperimentId, undefined, refresh)
 
+                                await breakpoint(5000)
+
                                 return {
                                     ...response,
                                     fakeInsightId: Math.random().toString(36).substring(2, 15),

@danielbachhuber danielbachhuber requested a review from a team January 29, 2025 23:22
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

This PR improves the experiment results loading experience by removing blocking animations and optimizing component rendering during loading states.

  • Removed ExperimentLoadingAnimation from frontend/src/scenes/experiments/ExperimentView/ExperimentView.tsx to prevent blocking UI during result calculations
  • Added result dependency to resize observer in frontend/src/scenes/experiments/MetricsView/DeltaChart.tsx to fix chart dimension updates
  • Delayed rendering of ExperimentImplementationDetails until results are fully loaded to avoid UI flashing
  • Optimized loading states to allow partial content display while results calculate

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

Comment on lines +97 to +101
<div className="xl:flex">
<div className="w-1/2 mt-8 xl:mt-0">
<DataCollection />
</div>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

style: The DataCollection div is set to w-1/2 but there's no complementary div using the other half of the space. This creates unbalanced layout.

Copy link
Contributor

github-actions bot commented Jan 29, 2025

Size Change: 0 B

Total Size: 1.16 MB

ℹ️ View Unchanged
Filename Size
frontend/dist/toolbar.js 1.16 MB

compressed-size-action

Copy link
Member

@raquelmsmith raquelmsmith left a comment

Choose a reason for hiding this comment

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

Amazing!

@danielbachhuber
Copy link
Contributor Author

@jurajmajerik I'll let you take a look at this before I merge, in case you have any opinions.

@danielbachhuber danielbachhuber changed the title feat(experiments): Avoid blocking results load animation feat(experiments): Remove blocking results load animation Jan 30, 2025
Copy link
Contributor

@jurajmajerik jurajmajerik left a comment

Choose a reason for hiding this comment

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

Great, thanks for moving quickly on this!

@danielbachhuber danielbachhuber merged commit 8b9f06a into master Jan 30, 2025
101 checks passed
@danielbachhuber danielbachhuber deleted the experiments/refactor-loading branch January 30, 2025 11:58
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