Overview

But both are less impressive due to the ~10 s delay between making changes and seeing those UI pieces react:

That's because both depend on the ~10s polling of /xb/api/autosaves/pending.

(The status badge changes from Published for an existing unmodified entity to Changed after you make any modification, as you can see in the GIF. See @jessebaker's diagram at #3505118-6: The status badge should indicate if there are changes to the page.)

Proposed resolution

We already are updating XB's preview immediately, by doing POST /xb/api/layout/…. after any client-side change.

That response currently only returns 

{"html": …}

We can just make that also return a list of auto-saves newly created during that request, that’d result in instantaneous updates to both the status badge and “Review changes”!

No websockets. Not even a new request 😊 We only need websockets for making those changes appear instantaneously for other users, but for those it’s fine if it only arrives every ~10s.

User interface changes

The same as in the GIF, but instantaneous!

CommentFileSizeAuthor
#13 glitches.gif248.61 KBhooroomoo
#11 XB UI instantaneous auto-save detecting.gif53.22 KBwim leers
XB UI polling dependent.gif167.72 KBwim leers

Issue fork experience_builder-3509270

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

wim leers created an issue. See original summary.

wim leers opened merge request !722

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: wim leers » lauriii
Status: Active » Needs review
Issue tags: +Needs product manager review

There: I propose to simply make mutations to the layout (POST or PATCH to /xb/api/layout/…) to change:

POST
                 type: object
                 required:
                   - html
+                  # For instantaneous updates upon making changes, in addition to polling /xb/api/autosaves/pending.
+                  - autosaves
                 properties:
                   html:
                     type: string
                     description: The HTML preview.
+                  autosaves:
+                    $ref: '#/components/schemas/AutoSaveCollection'
+                    description: 'For instantaneous updates upon making changes, in addition to polling /xb/api/autosaves/pending.'
PATCH
Note how this was already returning not just html, but also layout, model and entity_form_fields: 
@@ -1039,6 +1039,7 @@ paths:
                   - entity_form_fields
                   - model
                   - layout
+                  - autosaves
                 properties:
                   html:
                     type: string
@@ -1055,7 +1056,9 @@ paths:
                   entity_form_fields:
                     type: object
                     description: The full entity data.
-
+                  autosaves:
+                    $ref: '#/components/schemas/AutoSaveCollection'
+                    description: 'For instantaneous updates upon making changes, in addition to polling /xb/api/autosaves/pending.'
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

An alternate proposal

In preview.ts in previewApi.endpoints.postPreview.onQueryStarted trigger a clientside invalidation and force pendingChangesApi.endpoints.getAllPendingChanges to invalidate.

It would require adding a cache tag to getAllPendingChanges and using something like https://git.drupalcode.org/project/experience_builder/-/merge_requests/7... from onQueryStarted

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

#4: that works too, but does require an extra request. I know that the polling is still ongoing, and so this would just change the polling rhythm, if you will.

But … #4 is inevitably more concurrent requests immediately after making changes. And hence also more Drupal bootstraps.

I think in my proposal, we can even reduce the polling frequency, to say, every 30 seconds. It'll still feel instantaneous.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

Yes but it does require making use of RTK Query internals - https://redux-toolkit.js.org/rtk-query/usage/manual-cache-updates which as @balintbrews has pointed out

We recommend using automated re-fetching as a preference over manual cache updates in most situations.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

Just to be clear, I'm not against manual cache updates, PreviewEnvelope was designed for this purpose

larowlan opened merge request !723

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +Needs screenshots

So you're saying: your MR fixes it with a +13,0 diffstat, barely more than my OpenAPI-only change 🤣

Yeah, that's a no-brainer! Can't wait to test that tomorrow! 😄 (Would be happy to insta-merge after manual testing, and to move what I proposed to a new issue for a distant future.)

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: lauriii » wim leers

Will test @larowlan's MR after lunch 👍

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: wim leers » Unassigned
Priority: Normal » Major
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs product manager review, -Needs screenshots +sprint
StatusFileSize
new XB UI instantaneous auto-save detecting.gif53.22 KB

Well … that totally works 😄🤩

Still needs approval by somebody who knows RTK queries.

wim leers closed merge request !722

hooroomoo’s picture

hooroomoo
they/them
Location nyc🗽
commented
Issue summary: View changes
StatusFileSize
new glitches.gif248.61 KB

I may have found a regression so am looking into it. Clicking publish all changes shows the happy green smiley but then flashes back to show "Publish all changes" again

hooroomoo credited jessebaker.

hooroomoo’s picture

hooroomoo
they/them
Location nyc🗽
commented

Adding credit for @jessebaker for pointing me to the manualRetch that might not be necessary anymore and removing it fixed the regression i was looking at

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

@hooroomoo I've manually tested this and can't find any issues.. I can reproduce the delay on 0.x but with the MR in place it's gone, the Published > Changed label updates almost instantly. Publishing then works as expected - with no issues as in #13 - then I can repeat the whole cycle.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Related issues: +#3509509: When adding single hero component Auto-save shows a change when there is not one

I reverted @hooroomoo's changes after we tested in a zoom call and found the behaviour reported in #13 actually exists in HEAD too - #3509509: When adding single hero component Auto-save shows a change when there is not one

I've implemented the change of dispatch ordering per my review and that

a) fixes the bug from this issue
b) keeps things the same as they are in head for #3509509: When adding single hero component Auto-save shows a change when there is not one (i.e. it takes 10s for it to (wrongly) report 1 change - which is what was happening in #13 but immediately rather than after 10s

I think this is ready to go

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Reviewed & tested by the community » Fixed

See #11 for my enthusiasm after seeing the impact 🤩

  • wim leers committed 62c73b33 on 0.x authored by larowlan 
    Issue #3509270 by larowlan, hooroomoo, wim leers, jessebaker: Status...
effulgentsia’s picture

effulgentsia commented
Issue tags: -sprint

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.