feat: progressive loading of Analytics page #1447
Conversation
WalkthroughThis pull request introduces significant changes to the analytics page in the frontend, focusing on improving data fetching and loading state management. The modifications include restructuring the server-side data retrieval logic, adding new asynchronous functions for fetching metrics and counters, and implementing a loading spinner component. The changes aim to enhance error handling and provide a more robust user experience by adding loading states and improving data management. Changes
Possibly related PRs
Suggested labels
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
frontend/src/routes/(app)/(internal)/analytics/+page.server.ts (1)
108-110: Consider adding TypeScript types for the metrics data.The metrics data structure is used extensively in the UI. Adding type definitions would improve maintainability and catch potential type-related bugs early.
interface Metrics { controls: { total: number; active: number; deprecated: number; to_do: number; in_progress: number; on_hold: number; p1: number; eta_missed: number; }; compliance: { used_frameworks: number; audits: number; active_audits: number; progress_avg: number; non_compliant_items: number; evidences: number; }; risk: { assessments: number; scenarios: number; threats: number; acceptances: number; }; csf_functions: Array<{ name: string; value: number }>; audits_stats: { names: string[]; data: number[][]; uuids: string[]; }; } // Then use it in the return type stream: { metrics: Promise<Metrics> }frontend/src/routes/(app)/(internal)/analytics/+page.svelte (2)
20-20: Add TypeScript type for the metrics variable.The metrics variable and its reactive subscription look good, but adding type information would improve maintainability.
+import type { Metrics } from '$lib/types/analytics'; import LoadingSpinner from './LoadingSpinner.svelte'; -let metrics; +let metrics: Metrics | undefined;Also applies to: 32-41
150-334: Improve href values and loading state presentation.The conditional rendering and loading state implementation look good, but there are a few improvements that could enhance the user experience:
- Center the loading spinner and add a container:
- <div class="col-span-3 lg:col-span-1"> - <div>Refreshing data ..</div> - <LoadingSpinner /> - </div> + <div class="flex flex-col items-center justify-center p-8 space-y-4"> + <div class="text-lg font-semibold">Refreshing analytics data...</div> + <LoadingSpinner /> + </div>
- Fix the href for non-compliant items (currently "#"):
- href="#" + href="/compliance-assessments/?status=non_compliant"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/src/routes/(app)/(internal)/analytics/+page.server.ts(2 hunks)frontend/src/routes/(app)/(internal)/analytics/+page.svelte(3 hunks)frontend/src/routes/(app)/(internal)/analytics/LoadingSpinner.svelte(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/src/routes/(app)/(internal)/analytics/LoadingSpinner.svelte
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: migrations-check (3.12)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/src/routes/(app)/(internal)/analytics/+page.server.ts (2)
46-58: Enhance error handling and add type safety.While the basic error handling is good, consider these improvements:
- Add TypeScript types for better type safety
- Add HTTP status code checking
- Include more context in error messages
-const getMetrics = () => { +type MetricsResponse = { + results: unknown; +}; + +const getMetrics = async (): Promise<unknown> => { return fetch(`${BASE_API_URL}/get_metrics/`) .catch((error) => { - console.error('Failed to fetch metrics:', error); + console.error(`Failed to fetch metrics from ${BASE_API_URL}/get_metrics/:`, error); return { json: () => ({ results: null }) }; }) - .then((response) => response.json()) + .then((response) => { + if (!response.ok) { + throw new Error(`HTTP error! status: ${response.status}`); + } + return response.json() as Promise<MetricsResponse>; + }) .catch((error) => { console.error('Failed to parse metrics response:', error); return { results: null }; }) .then((data) => data.results); };
Line range hint
1-120: Consider architectural improvements for consistency and maintainability.The file would benefit from several architectural improvements:
- Apply consistent error handling across all fetch calls, similar to
getMetrics- Standardize on either
.then()chains orasync/awaitsyntax- Consider extracting repeated data transformations into utility functions
Example refactor for consistency:
// Create a typed fetch utility type ApiResponse<T> = { results: T; }; async function apiFetch<T>(endpoint: string): Promise<T | null> { return fetch(`${BASE_API_URL}${endpoint}`) .catch((error) => { console.error(`Failed to fetch ${endpoint}:`, error); return { json: () => ({ results: null }) }; }) .then((response) => { if (!response.ok) { throw new Error(`HTTP error! status: ${response.status}`); } return response.json() as Promise<ApiResponse<T>>; }) .catch((error) => { console.error(`Failed to parse ${endpoint} response:`, error); return { results: null }; }) .then((data) => data.results); } // Extract time state logic const TimeState = { INCOMING: { name: 'incoming', hexcolor: '#93c5fd' }, OUTDATED: { name: 'outdated', hexcolor: '#f87171' }, TODAY: { name: 'today', hexcolor: '#fbbf24' } } as const; function getTimeState(date: string) { const eta = new Date(date); if (eta.getTime() > TODAY.getTime()) { return TimeState.INCOMING; } else if (eta.getTime() < TODAY.getTime()) { return TimeState.OUTDATED; } return TimeState.TODAY; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/routes/(app)/(internal)/analytics/+page.server.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
frontend/src/routes/(app)/(internal)/analytics/+page.server.ts (1)
115-118: Verify streaming implementation and type safety.The streaming implementation might need additional safeguards:
- Consider adding error boundaries in the consuming component to handle streaming failures gracefully
- Add type definitions for the streamed metrics data to replace the removed interfaces
- Add tests to verify the streaming behavior, especially given the PR note about test interference
Let's check for existing error boundaries and types:
✅ Verification successful
Implementation verified - error handling and type safety are properly implemented
The streaming implementation includes:
- Server-side error handling for fetch failures
- Client-side promise handling with loading states
- Type safety through TypeScript interfaces
- Graceful degradation with loading spinner
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for error boundary components and type definitions echo "Searching for error boundary components..." rg -l "error.*boundary" frontend/src/ echo "Searching for metrics-related type definitions..." rg -l "type.*Metrics" frontend/src/Length of output: 281
Script:
#!/bin/bash # Broader search for error handling, types, and streaming implementation echo "Checking for error handling patterns..." rg "try.*catch|throw|Error" frontend/src/routes/\(app\)/\(internal\)/analytics/ -A 2 echo "Looking for type definitions..." rg "interface|type.*=|export type" frontend/src/routes/\(app\)/\(internal\)/analytics/ -A 2 echo "Examining the page component..." cat frontend/src/routes/\(app\)/\(internal\)/analytics/+page.svelte echo "Checking streaming implementation..." rg "getMetrics" frontend/src/routes/\(app\)/\(internal\)/analytics/ -A 5Length of output: 19040
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
frontend/src/routes/(app)/(internal)/analytics/+page.server.ts (2)
43-52: Enhance error handling in getCounters.While the basic error handling is good, consider these improvements:
- Check HTTP status codes
- Add type checking for the response
- Handle specific error types
const getCounters = async () => { try { const response = await fetch(`${BASE_API_URL}/get_counters/`); + if (!response.ok) { + throw new Error(`HTTP error! status: ${response.status}`); + } const data = await response.json(); + if (!data.results || typeof data.results !== 'object') { + throw new Error('Invalid response format'); + } return data.results; } catch (error) { + if (error instanceof TypeError) { + console.error('Network error:', error); + } else { console.error('Failed to fetch or parse counters:', error); + } return null; } };
119-122: Add type safety and error boundaries for streams.Consider these improvements:
- Define an interface for the stream object
- Add error boundaries in the UI component to handle failed streams gracefully
+interface DataStream { + metrics: Promise<MetricsData | null>; + counters: Promise<CountersData | null>; +} return { // ... other properties stream: { metrics: getMetrics(), counters: getCounters() - } + } as DataStream };frontend/src/routes/(app)/(internal)/analytics/+page.svelte (2)
32-33: Add type information for metrics variable.While
countershas proper typing,metricslacks type information. Consider adding an interface for the metrics data structure.+interface Metrics { + controls: { + total: number; + active: number; + deprecated: number; + to_do: number; + in_progress: number; + on_hold: number; + p1: number; + eta_missed: number; + }; + compliance: { + used_frameworks: number; + active_audits: number; + audits: number; + progress_avg: number; + non_compliant_items: number; + evidences: number; + }; + risk: { + assessments: number; + scenarios: number; + threats: number; + acceptances: number; + }; + csf_functions: any[]; // Replace 'any' with proper type + audits_stats: { + names: string[]; + data: number[][]; + uuids: string[]; + }; +} -let metrics; +let metrics: Metrics;
336-338: Improve loading and error states UI.The current implementation could be enhanced with better loading indicators and error states.
-<div class="col-span-3 lg:col-span-1"> - <div>Refreshing data ..</div> - <LoadingSpinner /> -</div> +<div class="col-span-3 lg:col-span-1"> + {#if metricsError} + <div class="text-red-500"> + <i class="fas fa-exclamation-circle"></i> + Failed to load metrics: {metricsError.message} + </div> + {:else} + <div class="text-gray-600"> + <LoadingSpinner /> + <span class="ml-2">Loading metrics data...</span> + </div> + {/if} +</div> -<LoadingSpinner /> +{#if countersError} + <div class="text-red-500"> + <i class="fas fa-exclamation-circle"></i> + Failed to load counters: {countersError.message} + </div> +{:else} + <div class="text-gray-600"> + <LoadingSpinner /> + <span class="ml-2">Loading statistics...</span> + </div> +{/if}Also applies to: 387-387
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/src/lib/components/utils/LoadingSpinner.svelte(1 hunks)frontend/src/routes/(app)/(internal)/analytics/+page.server.ts(2 hunks)frontend/src/routes/(app)/(internal)/analytics/+page.svelte(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/src/lib/components/utils/LoadingSpinner.svelte
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
frontend/src/routes/(app)/(internal)/analytics/+page.server.ts (1)
53-62: Apply the same error handling improvements to getMetrics.The same error handling improvements suggested for
getCountersshould be applied here for consistency.
| // Subscribe to the streamed metrics | ||
| $: { | ||
| if (data.stream?.metrics) { | ||
| data.stream.metrics.then((metricsData) => { | ||
| metrics = metricsData; | ||
| }); | ||
| } | ||
|
|
||
| if (data.stream?.counters) { | ||
| data.stream.counters.then((countersData) => { | ||
| counters = countersData; | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and loading state management to stream subscriptions.
The current implementation doesn't handle promise rejections or manage loading states explicitly.
+let metricsError: Error | null = null;
+let countersError: Error | null = null;
+let isLoading = true;
$: {
if (data.stream?.metrics) {
data.stream.metrics.then((metricsData) => {
metrics = metricsData;
+ }).catch((error) => {
+ console.error('Failed to load metrics:', error);
+ metricsError = error;
+ }).finally(() => {
+ isLoading = false;
});
}
if (data.stream?.counters) {
data.stream.counters.then((countersData) => {
counters = countersData;
+ }).catch((error) => {
+ console.error('Failed to load counters:', error);
+ countersError = error;
+ }).finally(() => {
+ isLoading = false;
});
}
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
frontend/src/routes/(app)/(internal)/analytics/+page.svelte (3)
122-124: Improve loading state message and styling.The loading state could be enhanced for better user experience:
- Use "Loading..." instead of "Refreshing data .." for consistency with common UI patterns
- Apply the same grid styling as the loaded state to prevent layout shifts
- <div class="col-span-3 lg:col-span-1"> - <div>Refreshing data ..</div> + <section id="summary" class="grid grid-cols-6 gap-2"> + <div class="col-span-6 flex items-center justify-center"> + <div>Loading...</div>
312-314: Apply consistent loading state pattern.Use the same enhanced loading state pattern as suggested for metrics:
- <div class="col-span-3 lg:col-span-1"> - <div>Refreshing data ..</div> - <LoadingSpinner /> + <section id="stats" class="flex justify-center items-center"> + <div class="text-center"> + <div class="mb-2">Loading...</div> + <LoadingSpinner /> + </div>
121-362: Consider creating a reusable loading state component.Both metrics and counters sections implement similar loading and error states. Consider extracting this pattern into a reusable component to improve maintainability and consistency.
Example implementation:
<!-- LoadingState.svelte --> <script lang="ts"> export let loading = false; export let error: Error | null = null; export let onRetry: () => void; </script> {#if loading} <div class="flex justify-center items-center"> <div class="text-center"> <div class="mb-2">Loading...</div> <LoadingSpinner /> </div> </div> {:else if error} <div class="flex justify-center items-center"> <div class="text-center"> <p class="text-red-500 mb-2">Failed to load data. Please try again.</p> <button class="btn variant-filled" on:click={onRetry}> Retry </button> </div> </div> {:else} <slot /> {/if}Usage:
- {#await data.stream.metrics} - <div class="col-span-3 lg:col-span-1"> - <div>Refreshing data ..</div> - <LoadingSpinner /> - </div> - {:then metrics} + <LoadingState + loading={!data.stream.metrics} + error={metricsError} + onRetry={() => window.location.reload()} + > <section id="summary" class="grid grid-cols-6 gap-2"> <!-- ... --> </section> + </LoadingState>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/routes/(app)/(internal)/analytics/+page.svelte(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
frontend/src/routes/(app)/(internal)/analytics/+page.svelte (1)
20-20: LGTM! Clean import of LoadingSpinner component.
| </div> | ||
| </section> | ||
| {:catch error} | ||
| <div>Data load eror</div> |
There was a problem hiding this comment.
Fix typo in error message and enhance error handling.
The error message contains a typo and lacks proper handling:
- <div>Data load eror</div>
+ <section id="stats" class="flex justify-center items-center">
+ <div class="text-center">
+ <p class="text-red-500 mb-2">Failed to load statistics. Please try again.</p>
+ <button class="btn variant-filled" on:click={() => window.location.reload()}>
+ Retry
+ </button>
+ </div>
+ </section>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div>Data load eror</div> | |
| <section id="stats" class="flex justify-center items-center"> | |
| <div class="text-center"> | |
| <p class="text-red-500 mb-2">Failed to load statistics. Please try again.</p> | |
| <button class="btn variant-filled" on:click={() => window.location.reload()}> | |
| Retry | |
| </button> | |
| </div> | |
| </section> |
| <div class="col-span-3 lg:col-span-1"> | ||
| <p class="text-red-500">Error loading metrics</p> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling with recovery options.
The error message should provide more context and a way to recover:
- <div class="col-span-3 lg:col-span-1">
- <p class="text-red-500">Error loading metrics</p>
+ <section id="summary" class="grid grid-cols-6 gap-2">
+ <div class="col-span-6 flex items-center justify-center">
+ <div class="text-center">
+ <p class="text-red-500 mb-2">Failed to load metrics. Please try again.</p>
+ <button class="btn variant-filled" on:click={() => window.location.reload()}>
+ Retry
+ </button>
+ </div>
+ </div>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div class="col-span-3 lg:col-span-1"> | |
| <p class="text-red-500">Error loading metrics</p> | |
| <section id="summary" class="grid grid-cols-6 gap-2"> | |
| <div class="col-span-6 flex items-center justify-center"> | |
| <div class="text-center"> | |
| <p class="text-red-500 mb-2">Failed to load metrics. Please try again.</p> | |
| <button class="btn variant-filled" on:click={() => window.location.reload()}> | |
| Retry | |
| </button> | |
| </div> | |
| </div> |
Note to self: could interfere with the tests
Summary by CodeRabbit
Release Notes
New Features
Improvements
Performance