Only load events table from the URL on allow list of URLs #6949

pauldambra · 2021-11-08T14:30:28Z

The events table was loading from URL on any URL which was causing issues in turbo mode.

This changes that behaviour to only load from URL on allow-listed URLs

mariusandra

Here's a proposal for improvement. Could you pass the Scene as a prop to the table logic, and have it smartly only react to the URL for that specific scene? Right now if you do have all three pages (actions, events, persons) open, all three logics will be running the update queries.

I also recommend using urls.events() instead of hardcoding /events whenever possible.

mariusandra · 2021-11-08T15:29:16Z

Ideally the logic could also check if it's on the URL it's supposed to be on, and only then reload the results.

mariusandra

Yep, that last change is exactly what I would have liked to see.

However having thought of it, I was a bit wrong earlier. It's not the case that we can have three different eventTableLogics running in the background, all trying to find properties in the URL. That's because eventsTableLogic is only exported as the scene logic for the events page. The person and action scene's logics don't connect to it directly, so it won't remain mounted if you move away from the page. We may want to change that later to persist the data, but it's like that now.

Just two things more:

Can you remove the now invalid comment, and the entire initialPathname code.
Slight scope creep, but still relevant a clean turbo mode fix: can you also pause the periodic event polling when the URL is not what the table wants it to be? A simple if inside the timeout is probably enough? You can possibly use the UrlPattern library that kea-router uses under the hood to make things compatible, and have :id work fine. Alternatively, pass the real person.id instead of :id into the sceneUrl and it should also work fine.

mariusandra · 2021-11-09T07:55:05Z

frontend/src/scenes/events/eventsTableLogic.ts

+                    // since this is a catch-all route, this code might run during or after the logic was unmounted
+                    // if we have an error accessing the filter value, the logic is gone and we should return


Hm... is this so? It's not really a catch-all route anymore with "props.sceneUrl"... and after a logic is unmounted, nothing here should run anymore. Wasn't this so when testing it?

It's a slow test loop, I thought it was necessary while testing but may have got my brain out of synch with the code :) I'll recheck

mariusandra · 2021-11-09T07:57:48Z

frontend/src/scenes/events/eventsTableLogic.test.ts

+        beforeLogic: () => {
+            router.actions.push('/events')
+        },


Makes sense and 👍... though (and out of scope for this PR) I've actually started to refactor tests with initKeaTestLogic into more explicit beforeEach(() => { initKeaTests(); someLogic.mount() } style.

This initKeaTestLogic function isn't good since it can't really be nested, and it always needs to be put in its own describe block even if you just have one test, since it runs beforeEach internally...

mariusandra · 2021-11-09T08:11:27Z

frontend/src/scenes/events/eventsTableLogic.ts

+            [props.sceneUrl]: (_: Record<string, any>, searchParams: Record<string, any>): void => {
+                try {
+                    // if the url changed, but we are not anymore on the page we were at when the logic was mounted
+                    if (router.values.location.pathname !== values.initialPathname) {


This initialPathname wouldn't work anymore with persons if the logic would remain mounted between scenes, as changing a person's ID will change the URL, and if this logic remains mounted, the initialPathname will point to somebody else... and the table will stop working.

Luckily we do unmount this logic for most persons, but there might be a day when we don't. I'd just remove this entire check. Now that you use props.sceneUrl, it doesn't make much sense anymore.

…urltoaction is now specific

pauldambra · 2021-11-09T10:44:55Z

@mariusandra either turbo mode isn't working on my machine or once we're keyed on url the logic "sleeps" when not loaded

…e as the action cannot be saved

mariusandra · 2021-11-09T11:24:28Z

When browsing through the redux state, it seems that there are two parallel copies of the events table loaded:

Fixing that by adding the sceneUrl prop to the scene export, it now properly loads stuff in the background as it should have all along:

So I made all the urls specific, and added an escape hatch into pollEvents.

I also removed the misleading tabs that were shown on the person and action events tables.

…og into events_table_url_to_action

pauldambra · 2021-11-09T15:42:33Z

see #6513

mariusandra · 2021-11-09T15:54:20Z

The tests were still failing here before the merge... and now I see identical tests failing in unrelated PRs that have merged with the latest master, e.g: #6993

pauldambra · 2021-11-09T15:58:43Z

The tests were still failing here before the merge...

😱

pauldambra · 2021-11-09T16:13:52Z

@mariusandra it's a bug that the automerge triggered when there were failing e2e tests?

The failing tests are (I hope!) unrelated to this change

I've pulled from master and run those failing tests locally and they pass, so I'm hoping we picked up other changes with those bugs that are now fixed in master

fuziontech · 2021-11-09T17:05:10Z

bin/plugin-server

@@ -37,7 +37,7 @@ if [[ -n $DYNO_RAM ]]; then
  export WORKER_CONCURRENCY=$(( ($DYNO_RAM - 1) / 512 + 1 ))
 fi

-cd plugins
+cd plugin-server


This change broke plugin server

oh wow, that wasn't there when I last reviewed it... 👀

No way! The test don't run locally for me without that change 😱

pauldambra added 2 commits November 8, 2021 14:28

only load events table from the URL on allow list of URLs

8fa7f26

add return type to function

202b77e

pauldambra requested a review from mariusandra November 8, 2021 14:41

mariusandra requested changes Nov 8, 2021

View reviewed changes

pass scene URL into events table

734535e

mariusandra requested changes Nov 9, 2021

View reviewed changes

pauldambra added 3 commits November 9, 2021 10:13

use urlpattern to check if scene is loaded in events table

bb40e98

don't need to check if still on the same page as when loaded because …

d856343

…urltoaction is now specific

which means we don't need url pattern matching at all

3f3bccf

mariusandra and others added 6 commits November 9, 2021 11:50

Merge branch 'master' into events_table_url_to_action

b83debc

Merge branch 'master' into events_table_url_to_action

b34b9bb

add scene url to events scene export's logic props

99ef483

use a different action name for each run or retries can never complet…

a9023cd

…e as the action cannot be saved

do not poll results if moved away from sceneUrl

3f2a378

remove tabs from person/action event tables

4c5b024

mariusandra approved these changes Nov 9, 2021

View reviewed changes

Merge branch 'events_table_url_to_action' of github.com:PostHog/posth…

6a697a3

…og into events_table_url_to_action

pauldambra force-pushed the events_table_url_to_action branch from 6a697a3 to aed455b Compare November 9, 2021 12:47

mariusandra force-pushed the events_table_url_to_action branch from aed455b to 6a697a3 Compare November 9, 2021 12:56

pauldambra added 3 commits November 9, 2021 13:15

so cypress can run locally

5f49bdb

break the dependency of running one test first so the other can pass

90d58a8

move check into logic and add an e2e test

336475a

pauldambra enabled auto-merge (squash) November 9, 2021 15:12

complete the rename refactoring to sceneIsEventsPage

2745c44

pauldambra merged commit 0cdfb25 into master Nov 9, 2021

pauldambra deleted the events_table_url_to_action branch November 9, 2021 15:35

mariusandra mentioned this pull request Nov 9, 2021

Change insight URL only if scene active #6993

Merged

fuziontech reviewed Nov 9, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only load events table from the URL on allow list of URLs #6949

Only load events table from the URL on allow list of URLs #6949

pauldambra commented Nov 8, 2021

mariusandra left a comment

mariusandra commented Nov 8, 2021

mariusandra left a comment

mariusandra Nov 9, 2021

pauldambra Nov 9, 2021

mariusandra Nov 9, 2021

mariusandra Nov 9, 2021

pauldambra commented Nov 9, 2021

mariusandra commented Nov 9, 2021

pauldambra commented Nov 9, 2021

mariusandra commented Nov 9, 2021

pauldambra commented Nov 9, 2021

pauldambra commented Nov 9, 2021

fuziontech Nov 9, 2021

fuziontech Nov 9, 2021

mariusandra Nov 9, 2021

pauldambra Nov 9, 2021

		// since this is a catch-all route, this code might run during or after the logic was unmounted
		// if we have an error accessing the filter value, the logic is gone and we should return

Only load events table from the URL on allow list of URLs #6949

Only load events table from the URL on allow list of URLs #6949

Conversation

pauldambra commented Nov 8, 2021

mariusandra left a comment

Choose a reason for hiding this comment

mariusandra commented Nov 8, 2021

mariusandra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pauldambra commented Nov 9, 2021

mariusandra commented Nov 9, 2021

pauldambra commented Nov 9, 2021

mariusandra commented Nov 9, 2021

pauldambra commented Nov 9, 2021

pauldambra commented Nov 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment