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 11 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions frontend/src/scenes/dashboard/DashboardItems.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ export function DashboardItems(): JSX.Element {
highlightedInsightId,
refreshStatus,
itemsLoading,
loadingRemainingTiles,
effectiveEditBarFilters,
urlVariables,
temporaryBreakdownColors,
Expand Down Expand Up @@ -250,6 +251,14 @@ export function DashboardItems(): JSX.Element {
})}
</ReactGridLayout>
)}
{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

<span>Loading remaining tiles...</span>
</div>
</div>
)}
</div>
)
}
83 changes: 77 additions & 6 deletions frontend/src/scenes/dashboard/dashboardLogic.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import { Link } from 'lib/lemon-ui/Link'
import { featureFlagLogic } from 'lib/logic/featureFlagLogic'
import { clearDOMTextSelection, getJSHeapMemory, shouldCancelQuery, toParams, uuid } from 'lib/utils'
import { DashboardEventSource, eventUsageLogic } from 'lib/utils/eventUsageLogic'
import { BREAKPOINTS } from 'scenes/dashboard/dashboardUtils'
import { calculateLayouts } from 'scenes/dashboard/tileLayouts'
import { dataThemeLogic } from 'scenes/dataThemeLogic'
import { MaxContextInput, createMaxContextHelpers } from 'scenes/max/maxTypes'
Expand Down Expand Up @@ -155,7 +156,12 @@ export const dashboardLogic = kea<dashboardLogicType>([
/**
* Dashboard loading and dashboard tile refreshes.
*/
loadDashboard: (payload: { action: DashboardLoadAction }) => payload,
loadDashboard: (payload: {
action: DashboardLoadAction
limitTiles?: number // optional limit for progressive loading
}) => payload,
/** Load remaining tiles after partial dashboard load. */
loadRemainingTiles: true,
/** Expose additional information about the current dashboard load in dashboardLoadData. */
loadingDashboardItemsStarted: (action: DashboardLoadAction) => ({ action }),
/** Expose response size information about the current dashboard load in dashboardLoadData. */
Expand Down Expand Up @@ -266,13 +272,19 @@ export const dashboardLogic = kea<dashboardLogicType>([
dashboard: [
null as DashboardType<QueryBasedInsightModel> | null,
{
loadDashboard: async ({ action }, breakpoint) => {
loadDashboard: async ({ action, limitTiles }, breakpoint) => {
actions.loadingDashboardItemsStarted(action)

await breakpoint(200)

try {
const apiUrl = values.apiUrl('force_cache', values.urlFilters, values.urlVariables)
const apiUrl = values.apiUrl(
'force_cache',
values.urlFilters,
values.urlVariables,
values.currentLayoutSize,
limitTiles
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could put this behind a feature flag.

)
const dashboardResponse: Response = await api.getResponse(apiUrl)
const dashboard: DashboardType<InsightModel> | null = await getJSONOrNull(dashboardResponse)

Expand All @@ -289,6 +301,29 @@ export const dashboardLogic = kea<dashboardLogicType>([
throw error
}
},
loadRemainingTiles: async (_, breakpoint) => {
if (!values.dashboard) {
return values.dashboard
}

await breakpoint(200)

try {
const apiUrl = values.apiUrl(
'force_cache',
values.urlFilters,
values.urlVariables,
values.currentLayoutSize
)
const dashboardResponse: Response = await api.getResponse(apiUrl)
const dashboard: DashboardType<InsightModel> | null = await getJSONOrNull(dashboardResponse)
return getQueryBasedDashboard(dashboard)
} catch (error: any) {
// If loading remaining tiles fails, just return current dashboard
console.warn('Failed to load remaining tiles:', error)
return values.dashboard
}
},
saveEditModeChanges: async (_, breakpoint) => {
actions.abortAnyRunningQuery()

Expand Down Expand Up @@ -724,6 +759,14 @@ export const dashboardLogic = kea<dashboardLogicType>([
loadDashboardFailure: () => false,
},
],
loadingRemainingTiles: [
false,
{
loadRemainingTiles: () => true,
loadRemainingTilesSuccess: () => false,
loadRemainingTilesFailure: () => false,
},
],
/** Dashboard variables */
initialVariablesLoaded: [
false,
Expand Down Expand Up @@ -924,15 +967,32 @@ export const dashboardLogic = kea<dashboardLogicType>([
return (
refresh?: RefreshType,
filtersOverride?: DashboardFilter,
variablesOverride?: Record<string, HogQLVariable>
variablesOverride?: Record<string, HogQLVariable>,
layoutSize?: 'sm' | 'xs',
limitTiles?: number
) =>
`api/environments/${teamLogic.values.currentTeamId}/dashboards/${id}/?${toParams({
refresh,
filters_override: filtersOverride,
variables_override: variablesOverride,
layout_size: layoutSize,
limit_tiles: limitTiles,
})}`
},
],
currentLayoutSize: [
(s) => [s.containerWidth],
(containerWidth): 'sm' | 'xs' => {
// Use precise container width when available, otherwise estimate from window width
if (containerWidth !== null) {
return containerWidth > BREAKPOINTS.sm ? 'sm' : 'xs'
}
// Estimate from window width, accounting for ~300px of sidebars
const windowWidth = typeof window !== 'undefined' ? window.innerWidth : 1200
const estimatedContainerWidth = windowWidth - 300
return estimatedContainerWidth > BREAKPOINTS.sm ? 'sm' : 'xs'
},
],
tiles: [(s) => [s.dashboard], (dashboard) => dashboard?.tiles?.filter((t) => !t.deleted) || []],
insightTiles: [
(s) => [s.tiles],
Expand Down Expand Up @@ -1139,7 +1199,7 @@ export const dashboardLogic = kea<dashboardLogicType>([
actions.loadDashboardSuccess(props.dashboard)
} else {
if (!(SEARCH_PARAM_QUERY_VARIABLES_KEY in router.values.searchParams)) {
actions.loadDashboard({ action: DashboardLoadAction.InitialLoad })
actions.loadDashboard({ action: DashboardLoadAction.InitialLoad, limitTiles: 4 })
}
}
}
Expand Down Expand Up @@ -1487,6 +1547,10 @@ export const dashboardLogic = kea<dashboardLogicType>([
return // We hit a 404
}

if (values.dashboard.has_more_tiles) {
actions.loadRemainingTiles()
}

if (values.placement !== DashboardPlacement.Export) {
// access stored values from dashboardLoadData
// as we can't pass them down to this listener
Expand All @@ -1499,6 +1563,13 @@ export const dashboardLogic = kea<dashboardLogicType>([
actions.reportDashboardViewed()
}
},
loadRemainingTilesSuccess: () => {
// Refresh all tiles using the same logic as initial dashboard load
if (values.placement !== DashboardPlacement.Export) {
const action = values.dashboardLoadData.action!
actions.refreshDashboardItems({ action, forceRefresh: false })
}
},
reportDashboardViewed: async (_, breakpoint) => {
// Caching `dashboard`, as the dashboard might have unmounted after the breakpoint,
// and "values.dashboard" will then fail
Expand Down Expand Up @@ -1591,7 +1662,7 @@ export const dashboardLogic = kea<dashboardLogicType>([
}

if (SEARCH_PARAM_QUERY_VARIABLES_KEY in router.values.searchParams) {
actions.loadDashboard({ action: DashboardLoadAction.InitialLoadWithVariables })
actions.loadDashboard({ action: DashboardLoadAction.InitialLoadWithVariables, limitTiles: 4 })
}

actions.setInitialVariablesLoaded(true)
Expand Down
1 change: 1 addition & 0 deletions frontend/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2141,6 +2141,7 @@ export type DashboardTemplateScope = 'team' | 'global' | 'feature_flag'

export interface DashboardType<T = InsightModel> extends DashboardBasicType {
tiles: DashboardTile<T>[]
has_more_tiles?: boolean
filters: DashboardFilter
variables?: Record<string, HogQLVariable>
persisted_filters?: DashboardFilter | null
Expand Down
55 changes: 51 additions & 4 deletions posthog/api/dashboards/dashboard.py
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,7 @@ def get_access_control_version(self, dashboard: Dashboard) -> str:

class DashboardSerializer(DashboardBasicSerializer):
tiles = serializers.SerializerMethodField()
has_more_tiles = serializers.SerializerMethodField()
filters = serializers.SerializerMethodField()
variables = serializers.SerializerMethodField()
created_by = UserBasicSerializer(read_only=True)
Expand Down Expand Up @@ -216,6 +217,7 @@ class Meta:
"data_color_theme_id",
"tags",
"tiles",
"has_more_tiles",
"restriction_level",
"effective_restriction_level",
"effective_privilege_level",
Expand Down Expand Up @@ -472,7 +474,7 @@ def get_tiles(self, dashboard: Dashboard) -> Optional[list[ReturnDict]]:

serialized_tiles: list[ReturnDict] = []

tiles = DashboardTile.dashboard_queryset(dashboard.tiles).prefetch_related(
tiles = DashboardTile.dashboard_queryset(dashboard.tiles.all()).prefetch_related(
Prefetch(
"insight__tagged_items",
queryset=TaggedItem.objects.select_related("tag"),
Expand All @@ -489,16 +491,25 @@ def get_tiles(self, dashboard: Dashboard) -> Optional[list[ReturnDict]]:
group_properties={"organization": {"id": str(team.organization_id)}},
)

# Get layout size and limit parameters for progressive loading
limit_tiles, layout_size = self._get_progressive_loading_params()

# Sort tiles by layout to ensure insights are computed in order of appearance on dashboard
# sm more common than xs, so we sort by sm
# Use the specified layout size to get the correct order for the current viewport
sorted_tiles = sorted(
tiles,
key=lambda tile: (
tile.layouts.get("sm", {}).get("y", 100),
tile.layouts.get("sm", {}).get("x", 100),
tile.layouts.get(layout_size, {}).get("y", 100),
tile.layouts.get(layout_size, {}).get("x", 100),
),
)

# 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 >= 10:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need/want this? We could just request a limit of 10 insights from the frontend.

sorted_tiles = sorted_tiles[:limit_tiles]
Comment on lines +510 to +511
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)


with task_chain_context() if chained_tile_refresh_enabled else nullcontext():
# Handle case where there are no tiles
if not sorted_tiles:
Expand All @@ -510,6 +521,42 @@ def get_tiles(self, dashboard: Dashboard) -> Optional[list[ReturnDict]]:

return serialized_tiles

def _get_progressive_loading_params(self):
"""Extract progressive loading parameters from request."""
request = self.context.get("request")
limit_tiles = None
layout_size = "sm"

if request and hasattr(request, "query_params"):
try:
limit_tiles = int(request.query_params.get("limit_tiles", ""))
except (ValueError, TypeError):
limit_tiles = None

layout_size = request.query_params.get("layout_size", "sm")
if layout_size not in ["sm", "xs"]:
layout_size = "sm" # fallback to sm if invalid value

return limit_tiles, layout_size

def get_has_more_tiles(self, dashboard: Dashboard) -> bool:
if self.context["view"].action == "list":
return False

# Check if tiles were limited
limit_tiles, _ = self._get_progressive_loading_params()

if limit_tiles is None or limit_tiles <= 0:
return False

# Get total number of tiles
tiles = DashboardTile.dashboard_queryset(dashboard.tiles.all())
total_tiles = tiles.count()

# Only indicate more tiles if there are >= 10 total tiles and more than the limit
# For dashboards with < 10 tiles, we return all tiles regardless of limit_tiles
return total_tiles >= 10 and total_tiles > limit_tiles

def get_filters(self, dashboard: Dashboard) -> dict:
request = self.context.get("request")
return filters_override_requested_by_client(request, dashboard)
Expand Down
Loading