fix(flow-client): loading state muting based on trigger events#24230
fix(flow-client): loading state muting based on trigger events#24230
Conversation
Fixes #24075 This change reverts the eager removal of loading state introduced by #23229, as it causes the indication to disappear during ongoing loading. As a replacement, it re-introduces debouncing tracking of active requests, and adds event-based silencing of the loading indication to avoid flashing the indicator for high-frequency UI interactions. In addition, instead of setting loading state using `ConnectionState.setState()` directly, the proper connection state methods (`loadingStarted()`, `loadingFinished()`) are used to avoid interference with loading state for requests from other sources outside Flow client.
|
| + pushPendingMessage.toJson()); | ||
| JsonObject payload = pushPendingMessage; | ||
| pushPendingMessage = null; | ||
| registry.getRequestResponseTracker().startRequest(); |
There was a problem hiding this comment.
Can you explain why the call to startRequest() can be removed?
Same in the send method.
There was a problem hiding this comment.
Having multiple calls to startRequest() on different branches of message sending code path is needlessly complex and confusing.
The sendPayload(final JsonObject payload) actually already takes care of calling startRequest() itself just before actually sending any message, regardless of the transport (XHR or push). This makes it an ideal single place candidate to keep for me.
In this instance, the sendPayload(payload) invoked below also call startRequest() if that was not done already, therefore making the call here is probably unnecessary.
In the send() case, one of the branches on the code path ends up calling startRequest() and then queueing the message for later instead of actually sending it with sendPayload(payload), which seems just inaccurate to me. Again, in that case, the queued message will get sent eventually through the sendPayload(payload) method, and during that time the startRequest() will be called by sendPayload(payload) itself.
Now that the code path is cleaned up a bit, it would rather make sense to me to remove if guarding against the “another request active” exception around startRequest() in sendPayload(payload), which feels like something added earlier to aid with multiple and sometimes missed startRequest() calls in various code path branches. Would that make sense?
There was a problem hiding this comment.
Probably it can be removed, but I have some concerns about the "reconnect" path.
mcollovati
left a comment
mcollovati
left a comment
There was a problem hiding this comment.
I did some tests locally and it seems to work fine. However, it is a bit challenging to properly reproduce the issue.
Do you think it would be possible to add some integration test for this? I guess it's not that easy.
| public void processMessage(String rpcType, String eventType) { | ||
| // Require at least one non-silent message to indicate loading for | ||
| // the next request. | ||
| boolean silent = JsonConstants.RPC_EVENT_TYPE.equals(rpcType) |
There was a problem hiding this comment.
Shouldn't it be RPC_TYPE_EVENT? The constants have the same "event" value, but RPC_EVENT_TYPE seems incorrect here.
2 participants
Fixes #24075
This change reverts the eager removal of loading state introduced by #23229, as it causes the indication to disappear during ongoing loading. As a replacement, it re-introduces debouncing tracking of active requests, and adds event-based silencing of the loading indication to avoid flashing the indicator for high-frequency UI interactions.
In addition, instead of setting loading state using
ConnectionState.setState()directly, the proper connection state methods (loadingStarted(),loadingFinished()) are used to avoid interference with loading state for requests from other sources outside Flow client.