Implement custom events suggestions

[zoldar]

Changes

Preview deployment available at https://pr-4092.review.plausible.io (access possible using staging credentials)

Slow connection can be emulated by executing liveSocket.enableLatencySim(900) in browser's JavaScript console.

Depends on #4033

The primary purpose of this PR is adding support for suggesting event names when defining custom goal events.

The secondary purpose (which turned out to be much more time consuming), is addressing a problem of live components embedded inside a modal perserving their state through close/open cycle when done fast enough. Live components do not have an explicit unmount phase in their lifecycle. Instead, they are pruned from LiveView state when they are no longer visible in DOM structure on the frontend, after applying an LV patch. The pruning process is done in 2 stages, with 2 roundtrips to the server and the actual removal from the backend state is done during the second one. If a given components becomes reachable in DOM again between those roundtrips, it is not going to be removed. My understanding is that it's not possible to tell with certianty that disappearance from DOM is fully intended or just the effect of intermediate patch application in rapid succession.

Initially, I have tried to find a way to ensure modal is reopened only after all the components inside it are removed from BE state. However, LV does not expose any way to inspect internal components state this way. I could only achieve that via a hack where I've eavesdropped process' state via :sys.get_state/1 in f34ceb7. Even though the solution worked, it was not acceptable for production use because :sys.get_state/1 shouldn't be used anywhere outside debugging code. Another important factor was longer wait time for components to disappear with periodic probing of state, which was pretty wasteful. I eventually went with that problem over to #phoenix at Elixir Slack (question asked there quoted below).

In the end, the only workable solution turned out to be making all embedded live components' IDs unique with every close/open cycle of the modal. That applies to every nested live component, unfortunately, as that "liveness issue" applies to all levels of component hierarchy. The modal generates a unique, predicatable modal ID changed with every close/open cycle. This ID should be appended to all live components' IDs inside the modal - unless state reset is implemented explicitly. The sequentiality of the unique ID makes it predicatable enough for testing purposes. One drawback of this approach is that component's IDs can't be determined outside modal component (so, for instance, send_update using ID from outside the modal might not be viable). However, if state is already explicitly manipulated from the outside, managing "reset" state explicitly is usually expected anyway.

I have also applied the same principle to fix the same "liveness" problem when switching tabs inside the modal. Every tab switch generates another sequential ID which is appended in top of sequential modal ID.

Quote of the original post on Slack follows:

Hello everyone 👋 I’m struggling with implementing a modal live component with a capability to nest arbitrary content inside, including other live components. The problems arise when live components with their own internal state are put inside such a modal. When hiding and showing an instance of a modal again, there’s an expectation that all its contents will be in default state. My current implementation tries to enforce a complete remount of the contents by removing the content rendered inside when the modal is closed and adding it again on reopening. Unfortunately, this does not play well with nested live components as they, with close/open cycle short enough, persist in their existing state. The removal of live components seems to be decided on the frontend after applying the patch with a sequence of cids_will_destroy and consecutive cids_destroyed (https://github.com/phoenixframework/phoenix_live_view/blob/02bc543fc2602a1de3f53dbfe2bbdfef443806ac/assets/js/phoenix_live_view/view.js#L1275-L1287) events getting pushed to the backend before they are completely removed from state.components of LV process. There’s currently no way to tell when the nested components are no longer present to guarantee they won’t be reused (instead of mounting them again) on rendering the surrounding content again. There’s socket.private.children_cids, but it seems to only contain CIDs which are pending rendering - and that does not count those currently unattached but still present components which can be brought back, given the proper timing. So far, I could only prototype a pretty egregious hack, where state.components are read out from LV process’ state dictionary and compared against initially recorded CIDs from socket.private.children_cids of the modal component. When the list of “live” cids becomes empty, the modal renders the content, ensuring all components inside are mounted again (f34ceb7).
This problem is especially evident on slower connections and seems to apply to any context where content with live components is conditionally rendered as togglable (for instance, with tabs implemented in LV in a similar fashion).
Now, I’m wondering, is that something that is so strongly against the way things are done in LV that it shouldn’t be done at all? Should such state “reset” of live components be explicitly managed throughout the whole live view hierarchy? Or perhaps would it be a legit reason for introduction some form of convenience in LV directly, by either keeping track of out-of-scope-but-still-alive components in each live component’s socket.private dict or exposing a function like LiveView.cid_exists?/1 (maybe something else)?
I sifted through the internals of LV and couldn’t find any sane way to achieve that. I have also tried some alternative approaches, like nesting modal contents in another live component with ID changed on each close/open cycle, but to no avail - the embedded components were still reinstated in their existing state and not remounted.

TODO

• fix modal not resetting combobox state when it should

Tests

• Automated tests have been added

Changelog

• This PR does not make a user-facing change

Documentation

• This change does not need a documentation update

Dark mode

• This PR does not change the UI

[aerosol]

So component state hacks aside, I think there's still some room for improvement in the auto-completion dialog - see attached screen recordings.

• Maybe the garbage we're seeing on staging could be filtered out with the hostname shield - in other words we should be serving only suggestions from events coming from allowed hostnames, similar to how it's done in Plausible.Stats.FilterSuggestions
• Sometimes upon selecting a goal name the spinner runs indefinitely - this is quite frequent, almost as if the suggestion content mattered in this case
• We get the impression that there's 25 events found during auto-discovery. Clicking the "add" link, and then re-opening the modal still shows plenty of suggestions but no "automatic addition" link anymore. Unclear why, related to discarded state maybe?

record-2024-06-10-11-01-28-vier.saul.pews.mp4

record-2024-06-10-11-15-04-debt.vase.faro.mp4

[zoldar]

Maybe the garbage we're seeing on staging could be filtered out with the hostname shield - in other words we should be serving only suggestions from events coming from allowed hostnames, similar to how it's done in Plausible.Stats.FilterSuggestions

That might work for native stats but imported custom events do not have hostname associated with them. Besides, that garbage is usually a result of security scans which AFAIK query valid hostnames too?

[zoldar]

Sometimes upon selecting a goal name the spinner runs indefinitely - this is quite frequent, almost as if the suggestion content mattered in this case

The values probably should be escaped when embedded in JS in combobox. I'll look into it.

[zoldar]

We get the impression that there's 25 events found during auto-discovery. Clicking the "add" link, and then re-opening the modal still shows plenty of suggestions but no "automatic addition" link anymore. Unclear why, related to discarded state maybe?

Automatic addition feature considers events that appeared in stats within last 6 months - and so do suggestions when adding a name. The problem is - the "automatic addition" query uses the default of first 25 results while rejecting all existing, configured goals from that list. The next time the autodetection is done, it will get the same list of those 25 first goals and reject all of them because they all are in the list already - filtering/rejection is done appside, after query results are fetched. I'll see how feasible it is to do it directly in the query. That was bollocks, names are already filtered out server-side - it's something to do with LV state.

[zoldar]

• Stopped limiting number of event names to add for autoconfigure in 9edb834, so that single auto-add link click adds all detected goals, not just first 25
• Started excluding detected suggested goals with whitespace at either end or consisting of whitespace entirely in 4aa9579
• Submit value in ComboBox is now JS escaped in 23c339a when passed to AlpineJS, to avoid crashes due to goal names containing characters like '

The PR is ready for another round of review \cc @aerosol @RobertJoonas

[aerosol]

Great! Almost there:

record-2024-06-10-13-58-31-awed.chao.duck.mp4

[zoldar]

@aerosol

Great! Almost there:

I can see that happening for any combobox input when you select the same suggestion the second time - also in prod 🙈 I'll see if I can do something about it however - maybe in a follow-up PR?