Issue 48715: Limit Data Region "Show all" to a maximum number of rows #5058
Conversation
| _getShowAllSelector(this).click(this.showAllRows.bind(this)); | ||
|
|
||
| if (this.maxRows === ALL_ROWS_MAX && this.totalRows > this.maxRows) { | ||
| this.addMessage('<span><b>Show all:</b> Displaying the first ' + ALL_ROWS_MAX.toLocaleString() + ' rows. Use paging to see more results.</span>'); |
There was a problem hiding this comment.
Certainly open to other messaging here. The main thing is I think we should display a message, otherwise, it just seems silly (borderline lying) when someone clicks "Show all" and they only see 1,000 rows.
There was a problem hiding this comment.
I agree. Having a message like this will be helpful to the user to know what's going on.
| @@ -3531,7 +3539,7 @@ if (!LABKEY.DataRegions) { | |||
|
|
|||
| msg += " " + "<span class='labkey-button select-none'>Select None</span>"; | |||
| var showOpts = []; | |||
| if (region.showRows !== 'all') | |||
| if (region.showRows !== 'all' && region.maxRows !== ALL_ROWS_MAX) | |||
There was a problem hiding this comment.
Reviewing this logic makes it apparent that are other ways to easily overburden the server/browser with results. The "Show Selected" and "Show Unselected" options should likely be limited in the same way.
There was a problem hiding this comment.
Yeah, good point. I'm curious how often those are used with large sets of rows though.
I played around with different values and it seems like anything over 5,000 is getting into sluggish page territory. Obviously, this is also influenced by the number of columns in the table but for the sake of this setting I'd just assume at least a few columns. Edit: I have increased the limit to 5,000 (Note: This PR is currently hung "Processing Updates" on GitHub...). |
| _getShowAllSelector(this).click(this.showAllRows.bind(this)); | ||
|
|
||
| if (this.maxRows === ALL_ROWS_MAX && this.totalRows > this.maxRows) { | ||
| this.addMessage('<span><b>Show all:</b> Displaying the first ' + ALL_ROWS_MAX.toLocaleString() + ' rows. Use paging to see more results.</span>'); |
There was a problem hiding this comment.
I agree. Having a message like this will be helpful to the user to know what's going on.
| @@ -3531,7 +3539,7 @@ if (!LABKEY.DataRegions) { | |||
|
|
|||
| msg += " " + "<span class='labkey-button select-none'>Select None</span>"; | |||
| var showOpts = []; | |||
| if (region.showRows !== 'all') | |||
| if (region.showRows !== 'all' && region.maxRows !== ALL_ROWS_MAX) | |||
There was a problem hiding this comment.
Yeah, good point. I'm curious how often those are used with large sets of rows though.
|
Screwed up merge. Closing in favor of #5067. |
Rationale
This addresses Issue 48715 by switching "Show all" functionality in Data Regions to set a
maxRows=1000parameter instead of settingshowRows=all. The intention is to a) avoid unwieldy large requests generated by our UI that can consume significant server resources and b) avoid crashing the user's browser attempting to render too many results. This is a client-side change only (soshowRows=allis still supported but just not generated by our UI).Related Pull Requests
Changes
DataRegion.showAllRows()to setting the URL parametermaxRows=1000instead ofshowRows=all