Bug #18661

Refresh Button makes the project's listing to flicker while it loads 2 times in a row

Added by Lucas Di Pentima 4 months ago. Updated 4 months ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
Workbench2
Start date:
02/01/2022
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
-
Release relationship:
Auto

Description

Once #18207 was merged, changing projects cleared the contents before populating it with the new data, and this a desirable behavior. Now that this listing clearing is in place, it has been made evident that the "Refresh" button is making Workbench2 to do extra work, seemingly loading the project twice because the listing flickers.

IIRC, the Refresh button requested the app to go back&forward the router's history, and this seemed like a good way to implement a generic reload feature instead of having the button signaling all the active parts of the app at any given time, but if it's making the app do extra requests and local work, it doesn't seem ideal.

Check how to get the same effect without simulating a going back in the app's router history.


Subtasks

Arvados - Task #18698: Review 18661-refresh-flicker-fixResolvedWard Vandewege


Related issues

Related to Arvados - Bug #18769: Test & confirm no stale data or flickering during data table refreshesResolved02/17/2022

Associated revisions

Revision d861bd54
Added by Lucas Di Pentima 4 months ago

Merge branch '18661-refresh-flicker-fix'. Closes #18661

Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <>

History

#1 Updated by Lucas Di Pentima 4 months ago

  • Status changed from New to In Progress

#2 Updated by Lucas Di Pentima 4 months ago

Updates at arvados-workbench2|bdb36daa - branch 18661-refresh-flicker-fix
Test run: developer-tests-workbench2: #573

The "Refresh" button doesn't go back & forth in the history, but just replaces the current location and that is a good solution. The double-loading behavior was recently introduced in #18195 but wasn't evident because the project's contents weren't cleared.

This one-line fix just avoids re-reloading the project because history.replace() already takes care of that.

#3 Updated by Ward Vandewege 4 months ago

Lucas Di Pentima wrote:

Updates at arvados-workbench2|bdb36daa - branch 18661-refresh-flicker-fix
Test run: developer-tests-workbench2: #573

The "Refresh" button doesn't go back & forth in the history, but just replaces the current location and that is a good solution. The double-loading behavior was recently introduced in #18195 but wasn't evident because the project's contents weren't cleared.

This one-line fix just avoids re-reloading the project because history.replace() already takes care of that.

LGTM, that was a short patch!

#4 Updated by Lucas Di Pentima 4 months ago

The related test failed, so the fix turned out not to be so simple.

I've been looking at the requests made when reloading projects, and it seems that the duplication indeed comes from the code that I removed on my last update but somehow is tightly coupled with the left side tree panel update code. I'm trying to see how to decouple both behaviors to avoid extra work and correctly refresh the left side panel at the same time.

Also, while reviewing the code for this, I've found some clues on why #18315 is happening. The issue has two parts: the main collection panel refresh issue is super easy to fix, but the collection files panel refresh issue may need more work, as I've seen traces of old code not being used anymore (from the old file browser component era).

#5 Updated by Peter Amstutz 4 months ago

  • Target version changed from 2022-02-02 sprint to 2022-02-16 sprint

#6 Updated by Peter Amstutz 4 months ago

  • Release set to 46

#7 Updated by Lucas Di Pentima 4 months ago

Updates at arvados-workbench2|606119d
Test run:

  • Fixes failing test by reordering action dispatching to make sure the app's store has fresh data before the side panel tree is rendered.
  • Improves test reliability by avoiding a potential race condition.
  • Fixes some unrelated HTML error on the DataTable component.

#8 Updated by Lucas Di Pentima 4 months ago

Updates at arvados-workbench2|1313e9c9
Test run:

Improves project loading further to immediately clear the table's contents upon load.
This is an additional improvement on #18207 and will probably be a good solution for #16951.

#9 Updated by Ward Vandewege 4 months ago

Lucas Di Pentima wrote:

Updates at arvados-workbench2|1313e9c9
Test run:

Improves project loading further to immediately clear the table's contents upon load.
This is an additional improvement on #18207 and will probably be a good solution for #16951.

Just one small comment: in src/store/workbench/workbench-actions.ts on line 447, a console.log call is added, is that leftover from debugging?

Otherwise, this LGTM, the difference when hitting "Refresh" is quite noticable, nicely done!

#10 Updated by Lucas Di Pentima 4 months ago

  • % Done changed from 0 to 100
  • Status changed from In Progress to Resolved

#11 Updated by Peter Amstutz 3 months ago

  • Related to Bug #18769: Test & confirm no stale data or flickering during data table refreshes added

Also available in: Atom PDF