Skip to content

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

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

feat: prog #37075

wants to merge 10 commits into from

Conversation

aspicer
Copy link
Contributor

@aspicer aspicer commented Aug 23, 2025

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

Copy link
Contributor

github-actions bot commented Aug 23, 2025

Size Change: +936 B (+0.04%)

Total Size: 2.66 MB

ℹ️ View Unchanged
Filename Size Change
frontend/dist/toolbar.js 2.66 MB +936 B (+0.04%)

compressed-size-action

@aspicer aspicer marked this pull request as ready for review August 23, 2025 06:38
@aspicer aspicer requested review from a team August 23, 2025 06:39
Copy link
Contributor

@greptile-apps greptile-apps bot left a 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

Edit Code Review Bot Settings | Greptile

actions.loadDashboard({
action: DashboardLoadAction.InitialLoad,
limitTiles: 4,
Copy link
Contributor

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,
Copy link
Contributor

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

Comment on lines +508 to +509
if limit_tiles is not None and limit_tiles > 0 and total_tiles >= 10:
sorted_tiles = sorted_tiles[:limit_tiles]
Copy link
Contributor

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

Suggested change
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" />
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant