-
Notifications
You must be signed in to change notification settings - Fork 1.9k
feat: prog #37075
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
feat: prog #37075
Conversation
Size Change: +936 B (+0.04%) Total Size: 2.66 MB ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4 files reviewed, 4 comments
actions.loadDashboard({ | ||
action: DashboardLoadAction.InitialLoad, | ||
limitTiles: 4, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Consider using a constant instead of hardcoded 4
for the initial tile limit
Context Used: Context - When defining constants, such as template IDs, use a constants file or define them at the top of the file for better maintainability. (link)
@@ -1607,6 +1678,7 @@ export const dashboardLogic = kea<dashboardLogicType>([ | |||
if (SEARCH_PARAM_QUERY_VARIABLES_KEY in router.values.searchParams) { | |||
actions.loadDashboard({ | |||
action: DashboardLoadAction.InitialLoadWithVariables, | |||
limitTiles: 3, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Different hardcoded limit (3) used here versus initial load (4) - consider consistency or document why they differ
if limit_tiles is not None and limit_tiles > 0 and total_tiles >= 10: | ||
sorted_tiles = sorted_tiles[:limit_tiles] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: The 10-tile threshold is hardcoded. Consider using a constant at the top of the file for better maintainability
if limit_tiles is not None and limit_tiles > 0 and total_tiles >= 10: | |
sorted_tiles = sorted_tiles[:limit_tiles] | |
# Progressive loading threshold - minimum tiles needed to enable progressive loading | |
PROGRESSIVE_LOADING_MIN_TILES = 10 | |
# Apply tile limit if specified, but only if there are enough tiles to make it worthwhile | |
# For dashboards with < 10 tiles, return all tiles even if limit_tiles is specified | |
total_tiles = len(sorted_tiles) | |
if limit_tiles is not None and limit_tiles > 0 and total_tiles >= PROGRESSIVE_LOADING_MIN_TILES: | |
sorted_tiles = sorted_tiles[:limit_tiles] |
Context Used: Context - When defining constants, such as template IDs, use a constants file or define them at the top of the file for better maintainability. (link)
{loadingRemainingTiles && ( | ||
<div className="mt-4 flex items-center justify-center"> | ||
<div className="flex items-center gap-2 text-muted"> | ||
<div className="animate-spin rounded-full h-4 w-4 border-b-2 border-blue-600" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Consider using a consistent loading spinner component instead of inline styles for better maintainability
Problem
Large dashboards load slowly, mainly because of the number of tiles.
Changes
Add progressive loading. Pretty naive implementation: the FE requests 4 tiles. If the backend finds the dashboard has 10 or less, it returns them all. Otherwise it sends the number requested. Then the FE fires off another query for everything.
Will increase time until the page is completely ready by about 0.5 seconds, but have data up in less time.
Screencast.From.2025-08-22.23-30-56_compressed.mp4
My local tests showed something like:
master: 4.5 seconds to load
this branch: 2.5 seconds to first graphs appearing, another 2.5 seconds until the dashboard was fully loaded.
How did you test this code?
Locally