appsmithorg / appsmith

Platform to build admin panels, internal tools, and dashboards. Integrates with 25+ databases and any API.
https://www.appsmith.com
Apache License 2.0
34.74k stars 3.76k forks source link

[Bug]-[4000]:Table Widget Skeleton should not disappear till the data is loaded #12308

Open danciaclara opened 2 years ago

danciaclara commented 2 years ago

Is there an existing issue for this?

Summary

On the deployed app, there is no way to indicate that the queries/functions are loading as the user looks at the page with blank widgets. On the edit mode, we have a spinner at the top but this doesn't show in the deployed version. Would be nice to have the spinner on the top bar or have the cursor change to a clock icon.

Why should this be worked on?

Suggestion by a discord user - https://discord.com/channels/725602949748752515/760761686549463060/957322085242384446

Nikhil-Nandagopal commented 2 years ago

@danciaclara the skeleton loading state is what indicates this right? Cursor is not a good experience because it means the entire application is unresponsive and loading which is never the case. The respective widgets going into loading state seems to be the right UX to me so I'm closing this but please do re-open with more information if you think I missed something

danciaclara commented 2 years ago

@Nikhil-Nandagopal Yeah that makes sense about the cursor. However, the skeleton screen disappears too quickly when the application is a bit heavy. It takes a few seconds for the table to populate after the skeleton screen disappears. Probably that caused the user to ask for the loading state as the skeleton screen went unnoticed. For eg: Here's a screenshot of the page showing the table without any data. Screenshot 2022-03-29 at 2 09 58 PM

Be it the skeleton screen or a circular loading state for individual widgets (as different widgets load times may differ based on the query) the data should appear immediately after the loading state.

Nikhil-Nandagopal commented 2 years ago

@danciaclara yeah that makes sense. This is a performance problem and the loading state should not change till the evaluation is complete

dilippitchika commented 2 years ago

I think this is a problem only with table widget, but not with any other widgets. Does anyone have any evidence for other widgets on this?

dilippitchika commented 2 years ago

Needs triaging:

We need to understand if this is happening because of any code in the table widget or because of the evaluations.

Please check our a-force app to understand the empty table issue.

dilippitchika commented 2 years ago

Reach - 303 Impact - High

sbalaji1192 commented 2 years ago

@dilippitchika This looks like due to evaluations. filteredTableData used by the table to render the body is a derived property and requires an evaluation cycle. but the header is generated from primary columns which is part of DSL and doesn't require an evaluation cycle.

CC: @Rishabh-Rathod

dilippitchika commented 2 years ago

@sbalaji1192 then is it possible to delay the loading of the header as well till evaluations are complete?

sbalaji1192 commented 2 years ago

@dilippitchika Table doesn't know whether the filteredTableData is not computed or is empty (tableData is empty). so we cannot make that decision inside the table widget. A possible solution would be to not set the loading state to false till the evaluation is completed. @eco-monk need you insight here.

dilippitchika commented 2 years ago

Stats

Stat Values
Reach 1500
Effort (months) 0.75
eco-monk commented 2 years ago

@sbalaji1192. We're showing loading state (not through isLoading prop) till evaluations are done for a widget. editorSelectors.tsx -> getCanvasWidgetDsl

isLoading property is set only when a query/api is triggered -> which is bound to the widget data.


The empty rows case happens when

  1. The table data is empty (should be addressed by #12709)
  2. The table schema is evaluated but the API is not triggered yet.

For the case where is API is not triggered.

  1. It can be the case where the API.run() is bound to a button click or some other action, that the user has to perform.
    • In this case, we shouldn't be setting the loading state pre-emptively. But show the No data available
  2. There's something we can do for on page load actions though (which seems to be the case with A-force)
    • Created a task #12869
sbalaji1192 commented 2 years ago

@eco-monk Thanks for the detailed explanation!

dilippitchika commented 2 years ago

Bumping to high for the following reasons

  1. When the data is huge or the evaluations are slow, the app viewer is unable to understand why they are still seeing older data
  2. There's no way for app developer to fix this by using something like a progress bar widget and update the user that evaluations are pending.

One suggestion Balaji had was to add an isEvaluating flag on our widgets. To begin with only Table widget can have this.

sbalaji1192 commented 2 years ago

@dilippitchika @eco-monk Suggested isEvaluating flag will be true when the query has been completed but the derived properties are not computed yet. This flag can be used by the widget itself to show a loader, or the User can show a progress bar until properties are computed.

dilippitchika commented 2 years ago

@sbalaji1192 i think something is evaluating should not be conveyed to a developer, for them all of it should be one property - query loading + table evaluating. What do you think? Can we improve the DX here?

sbalaji1192 commented 2 years ago

@dilippitchika That should also work. There is no need to differentiate between query is loading vs table data is getting evaluated. To the end user, both should be conveyed in the same way. that data is not ready yet. So having one flag for both is totally fine.

dilippitchika commented 2 years ago

Understood, can we use something like Table.isLoading property to handle both of these?

ashit-rath commented 2 years ago

I just noticed the same while triaging https://github.com/appsmithorg/appsmith/issues/18105 in List v2. The experience can be a bit jarring here as we see the old data for a flash and then see the new data. Although I am thinking of doing a monkey patch in the List widget but having a proper isLoading to handle both should be the right way.

@dilippitchika @somangshu This issue seems to be tagged for App Viewers and UI Builders, shouldn't this be assigned to FE-Coders?

somangshu commented 2 years ago

I agree Ashit. @satbir121 we need help here from you guys. The way isLoading behaves is the reason this experience exist. So you have suggestions? LMK what you think

rishabhrathod01 commented 2 years ago

@somangshu @ashit-rath

The current logic to determine the widget's loading state ( WidgetLoadingSaga/setWidgetsLoadingSaga ) works as follows

If we want to show the initial isLoading state for TableWidget until tableData is evaluated, we could show isLoading for all widgets until the 1st evaluation is not completed as after that tableData would be evaluated.

We could try adding an isInitialEvaluationInProgress redux state that could be consumed in the above setWidgetsLoadingSaga to set all widgets that has any dependency to isLoading.

cc: @sbalaji1192

Nikhil-Nandagopal commented 1 year ago

@Rishabh-Rathod but its not just about first load, it's also if the API is running via a button, the table stops loading once the API is complete, but the table doesn't render because the evaluation is not yet complete.

dilippitchika commented 1 year ago

@satbir121 assigning to you and team, i think this is a big impact on perceived performance as all our widgets have to wait until evals are also complete to show the data.

People using the isLoading binding find it hard to figure out what is going wrong with the table and how can we improve it. The same reason, animate loading property doesn't work as expected, because it doesn't take evals into consideration. @somangshu if you also agree that this is a perceived performance issue, can you please get this prioritised as a perf improvement?

dilippitchika commented 1 year ago

@bharath31 created a new issue as a dependency for this issue - https://github.com/appsmithorg/appsmith/issues/20600

rahulbarwal commented 1 month ago

Reopening, as the PR #36857 provides an alternative solution to the original problem and not completely fixes it.