Show the Benchmark Report link in FinOps Benchmark Banner when benchmark has succeeded#2043
Show the Benchmark Report link in FinOps Benchmark Banner when benchmark has succeeded#2043
Conversation
…ark has succeeded
There was a problem hiding this comment.
Pull request overview
This PR updates the Home FinOps Benchmark banner behavior so that when a benchmark run has SUCCEEDED, the UI can switch the banner into a “report-ready” variation and provide a navigation path to the benchmark report.
Changes:
- Added a shared
BenchmarkProvidertype derived fromBenchmarkProviders. - Introduced
useBenchmarkBannerState(plus tests) to fetch benchmarks and compute bannervariation+provider. - Updated Home onboarding/operational views to pass
variation,provider, and a “view report” click handler intoFinOpsBenchmarkBanner. - Added
listBenchmarks()to the React UI benchmark API client.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/shared/src/lib/benchmark/benchmark-providers.ts | Adds BenchmarkProvider type derived from enum values. |
| packages/react-ui/src/app/features/home/components/useBenchmarkBannerState.ts | New hook that queries benchmarks and selects banner variation/provider. |
| packages/react-ui/src/app/features/home/components/useBenchmarkBannerState.test.ts | Unit tests for the new banner-state hook logic. |
| packages/react-ui/src/app/features/home/components/home-operational-view.tsx | Wires the new hook into the operational home view and passes banner props + report navigation handler. |
| packages/react-ui/src/app/features/home/components/home-onboarding-view.tsx | Wires the new hook into the onboarding home view and passes banner props + report navigation handler. |
| packages/react-ui/src/app/features/benchmark/benchmark-api.ts | Adds listBenchmarks() endpoint client method. |
Comments suppressed due to low confidence (5)
packages/react-ui/src/app/features/benchmark/benchmark-api.ts:48
listBenchmarkstakesprovider?: string, but the backend querystring is validated againstBenchmarkProvidersand the rest of the feature treats provider as an enum-like value. Usingstringhere makes it easy to pass invalid providers at compile time.
Suggestion: type this parameter as BenchmarkProvider/BenchmarkProviders (from @openops/shared) and keep the function signature consistent with the server contract.
const listBenchmarks = (provider?: string): Promise<ListBenchmarksResponse> =>
api.get<ListBenchmarksResponse>(
'/v1/benchmarks',
provider ? { provider } : undefined,
);
packages/react-ui/src/app/features/home/components/home-operational-view.tsx:55
- The dashboard query string is built via string interpolation. Since
benchmarkProviderultimately comes from API data, it’s safer and more robust to URL-encode the value (or usecreateSearchParams) to avoid malformed URLs if the provider ever contains unexpected characters.
Suggestion: build the URL as /analytics?dashboard=${encodeURIComponent(${benchmarkProvider}_benchmark)} (or equivalent with createSearchParams).
const onViewBenchmarkReportClick = () =>
navigate(`/analytics?dashboard=${benchmarkProvider}_benchmark`);
packages/react-ui/src/app/features/home/components/home-onboarding-view.tsx:109
- The dashboard query string is built via string interpolation. Since
benchmarkProviderultimately comes from API data, it’s safer and more robust to URL-encode the value (or usecreateSearchParams) to avoid malformed URLs if the provider ever contains unexpected characters.
Suggestion: build the URL as /analytics?dashboard=${encodeURIComponent(${benchmarkProvider}_benchmark)} (or equivalent with createSearchParams).
const onViewBenchmarkReportClick = () =>
navigate(`/analytics?dashboard=${benchmarkProvider}_benchmark`);
packages/react-ui/src/app/features/home/components/useBenchmarkBannerState.test.ts:140
- This test uses
provider: 'azure', butBenchmarkProviders(and the server routes that validate providers) currently only support'aws'. That makes the scenario unreachable in production today and can be confusing if the test suite implies Azure benchmarks are supported.
Either update the test to only use supported providers, or (if Azure is intended) expand BenchmarkProviders/server-side support and dashboard registry so the behavior is actually possible end-to-end.
it('returns report variation when there are multiple benchmarks and one has SUCCEEDED', () => {
mockUseShowBenchmarkBanner.mockReturnValue(true);
setupQueryMock([
{ benchmarkId: 'bm-1', provider: 'aws', status: BenchmarkStatus.RUNNING },
{
benchmarkId: 'bm-2',
provider: 'azure',
status: BenchmarkStatus.SUCCEEDED,
},
]);
const { result } = renderHook(() => useBenchmarkBannerState());
expect(result.current.variation).toBe('report');
expect(result.current.provider).toBe('azure');
});
packages/react-ui/src/app/features/home/components/useBenchmarkBannerState.ts:29
provideris derived from astringcoming fromListBenchmarksResponseand then forced intoBenchmarkProvidervia a cast, with a hard-coded'aws'fallback. This can mask unexpected provider values (e.g., would build links like/analytics?dashboard=undefined_benchmark/ render an undefined label) and makes it easy for the UI and shared types to drift.
Consider (a) avoiding the cast by aligning the response type to BenchmarkProviders/BenchmarkProvider (e.g., schema uses Type.Enum(BenchmarkProviders)), and/or (b) validating succeededBenchmark.provider against the supported providers and falling back to BenchmarkProviders.AWS when unknown (instead of 'aws').
return {
isEnabled,
variation: succeededBenchmark ? 'report' : 'default',
provider: (succeededBenchmark?.provider ?? 'aws') as BenchmarkProvider,
};
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
923fed7 to
72986cb
Compare
| updated: string; | ||
| projectId: string; | ||
| provider: string; | ||
| provider: BenchmarkProviders; |
There was a problem hiding this comment.
This is a TypeScript-only change — it doesn't touch the TypeORM column definition (line 43–44 still has type: String). TypeORM reads the column as a plain string from the DB; no migration is triggered, no DB constraint is added.
|



Fixes OPS-3667.
Screenshots: