feat(ui): fetch storage capabilities from backend during Vite dev mode #3544
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,10 @@ import monitorImg from './media/monitor.png'; | |
| const MonitorATMEmptyState: React.FC = () => { | ||
| const config: MonitorEmptyStateConfig = getConfigValue('monitor.emptyState'); | ||
|
|
||
| if (!config) { | ||
| return null; | ||
| } | ||
|
Comment on lines
+14
to
+16
There was a problem hiding this comment. The new null check on lines 14-16 adds a code path that returns early when config is null/undefined. However, the existing test file (index.test.js) only tests the case where config is defined and doesn't cover the scenario where getConfigValue returns null/undefined. Consider adding a test case to verify that the component renders null when config is not available, ensuring this defensive code path is covered. |
||
|
|
||
| return ( | ||
| <Col> | ||
| <Row justify="center"> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| // Copyright (c) 2025 The Jaeger Authors. | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| // Fields that should be merged with user-supplied config values rather than overwritten. | ||
| export type TMergeField = 'dependencies' | 'search' | 'tracking' | 'monitor'; | ||
| export const mergeFields: readonly TMergeField[] = ['dependencies', 'search', 'tracking', 'monitor']; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -119,9 +119,9 @@ const defaultConfig: Config = { | |
| useOpenTelemetryTerms: false, | ||
| }; | ||
|
|
||
| import { mergeFields } from './config-keys'; | ||
|
There was a problem hiding this comment. The import statement at line 122 is placed after the const declaration, which is unconventional. While JavaScript/TypeScript hoists imports automatically, it's more idiomatic and consistent with typical code style to place all imports at the top of the file (lines 3-10). Consider moving this import to line 11 to group it with other imports. |
||
|
|
||
| export { mergeFields }; | ||
|
|
||
| export default deepFreeze(defaultConfig); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -8,11 +8,12 @@ import legacy from '@vitejs/plugin-legacy'; | |||||
| import path from 'path'; | ||||||
| import fs from 'fs'; | ||||||
| import { fileURLToPath } from 'url'; | ||||||
| import { mergeFields } from './src/constants/config-keys'; | ||||||
|
|
||||||
| const __dirname = path.dirname(fileURLToPath(import.meta.url)); | ||||||
|
|
||||||
| const proxyConfig = { | ||||||
| target: 'http://127.0.0.1:16686', | ||||||
|
There was a problem hiding this comment. The change from 'localhost' to '127.0.0.1' improves consistency with the fetch URL used later in the code (line 63), but breaks consistency with existing documentation. The CONTRIBUTING.md and AGENTS.md files both reference 'http://localhost:16686' throughout. While both localhost and 127.0.0.1 generally work the same, using localhost is more standard and user-friendly in documentation. Consider either reverting this change or updating the documentation files to match.
Suggested change
|
||||||
| secure: false, | ||||||
| changeOrigin: true, | ||||||
| ws: true, | ||||||
|
|
@@ -39,52 +40,124 @@ function jaegerUiConfigPlugin() { | |||||
|
|
||||||
| return { | ||||||
| name: 'jaeger-ui-config', | ||||||
| apply: 'serve' as const, | ||||||
| configureServer(server: import('vite').ViteDevServer) { | ||||||
| server.watcher.add([jsConfigPath, jsonConfigPath]); | ||||||
| server.watcher.on('change', (changedPath: string) => { | ||||||
| if (changedPath === jsConfigPath || changedPath === jsonConfigPath) { | ||||||
| console.log(`[jaeger-ui-config] Config changed: ${changedPath}. Triggering full reload...`); | ||||||
| server.ws.send({ type: 'full-reload', path: '*' }); | ||||||
| } | ||||||
| }); | ||||||
| }, | ||||||
| transformIndexHtml: { | ||||||
| order: 'pre' as const, | ||||||
| async handler(html: string) { | ||||||
|
Comment on lines
41
to
55
There was a problem hiding this comment. The |
||||||
| let backendUiConfig: any = null; | ||||||
| let storageCapabilities: any = null; | ||||||
| let version: any = null; | ||||||
|
Comment on lines
+56
to
+58
There was a problem hiding this comment. The variables 'backendUiConfig', 'storageCapabilities', and 'version' are all typed as 'any', which bypasses TypeScript's type safety. Since '@types/node' was added as a dependency in this PR, consider defining proper TypeScript interfaces for the expected backend response structure. This would make the code more maintainable and catch potential type errors at compile time.
Comment on lines
+56
to
+58
There was a problem hiding this comment. The variables interface BackendConfig {
uiConfig?: Record<string, any>;
storageCapabilities?: { archiveStorage?: boolean; metricsStorage?: boolean };
version?: { gitCommit: string; gitVersion: string; buildDate: string };
}
let backendConfig: BackendConfig | null = null; |
||||||
|
|
||||||
| const controller = new AbortController(); | ||||||
| const timeout = setTimeout(() => controller.abort(), 1000); | ||||||
|
There was a problem hiding this comment. The timeout value (1000ms) is hardcoded in the implementation. This should be extracted as a named constant at the top of the function or module level to make it more maintainable and easier to adjust if needed. Consider defining something like |
||||||
|
|
||||||
| try { | ||||||
| const fetchOptions = { signal: controller.signal }; | ||||||
| const response = await fetch('http://127.0.0.1:16686/api/ui/config', fetchOptions); | ||||||
|
|
||||||
|
Comment on lines
53
to
+66
There was a problem hiding this comment. This plugin is included unconditionally in the Vite config, and |
||||||
| if (response?.ok) { | ||||||
|
There was a problem hiding this comment. The optional chaining operator 'response?.ok' is unnecessary here. If the fetch call succeeds without throwing, the response object is guaranteed to be defined. The check can be simplified to 'response.ok'. The optional chaining doesn't add safety since any network or timeout errors are already caught in the catch block.
Suggested change
|
||||||
| const data = await response.json(); | ||||||
|
|
||||||
| // Extract data from unified response | ||||||
| storageCapabilities = data.storageCapabilities; | ||||||
| version = data.version; | ||||||
| backendUiConfig = data.uiConfig; | ||||||
|
Comment on lines
+67
to
+73
There was a problem hiding this comment. Consider adding validation for the backend response to ensure it has the expected structure before attempting to access properties. For example, if |
||||||
|
|
||||||
| console.log('[jaeger-ui-config] Fetched config from backend /api/ui/config'); | ||||||
|
Comment on lines
+63
to
+75
There was a problem hiding this comment. This code assumes the backend endpoint '/api/ui/config' exists and returns a JSON response with 'storageCapabilities', 'version', and 'uiConfig' fields. However, this is a frontend-only repository and the backend implementation is not visible in this PR. The PR description mentions this endpoint as a "prerequisite", but there's no verification that it exists or that it returns data in the expected format. If the endpoint doesn't exist yet or returns a different structure, the fetch will fail silently and the UI will fall back to defaults. Consider adding a comment documenting the expected response structure, or better yet, creating a TypeScript interface for the expected response. |
||||||
| } | ||||||
|
Comment on lines
+67
to
+76
There was a problem hiding this comment. The optional chaining Consider changing this to check only |
||||||
| } catch (err) { | ||||||
| // Silent fallback for production behavior, but log for dev visibility | ||||||
|
Comment on lines
+56
to
+78
There was a problem hiding this comment.
|
||||||
| if (err instanceof Error && err.name === 'AbortError') { | ||||||
| console.log('[jaeger-ui-config] Backend fetch timed out (1s)'); | ||||||
| } else if (err instanceof Error) { | ||||||
| console.log(`[jaeger-ui-config] Error fetching backend config: ${err.message}`, err); | ||||||
| } else { | ||||||
| console.log('[jaeger-ui-config] Unknown error fetching backend config:', err); | ||||||
| } | ||||||
|
Comment on lines
+77
to
+85
Contributor
There was a problem hiding this comment. The The code will incorrectly log "Backend not running or error fetching config" instead of "Backend fetch timed out (1s)" when a timeout occurs. Fix: try {
const fetchOptions = { signal: controller.signal };
const response = await fetch('http://127.0.0.1:16686/api/ui/config', fetchOptions);
if (response?.ok) {
const data = await response.json();
// ... rest of the logic
}
} catch (err) {
if (err instanceof Error && err.name === 'AbortError') {
console.log('[jaeger-ui-config] Backend fetch timed out (1s)');
} else {
console.log('[jaeger-ui-config] Backend not running or error fetching config');
}
}Remove the Spotted by Graphite Agent
Comment on lines
+77
to
+85
There was a problem hiding this comment. Backend fetch failures/timeouts are logged inside |
||||||
| } finally { | ||||||
| clearTimeout(timeout); | ||||||
| } | ||||||
|
|
||||||
| // Check for JS config first (highest priority - no merge) | ||||||
| if (fs.existsSync(jsConfigPath)) { | ||||||
| try { | ||||||
| const jsContent = fs.readFileSync(jsConfigPath, 'utf-8'); | ||||||
| const uiConfigFn = `function UIConfig() { ${jsContent} }`; | ||||||
| html = html.replace('// JAEGER_CONFIG_JS', uiConfigFn); | ||||||
| console.log( | ||||||
| '[jaeger-ui-config] Loaded config from jaeger-ui.config.js (full override, no merge)' | ||||||
| ); | ||||||
| // Note: Don't return early here, we still need to inject storageCapabilities and version below | ||||||
|
Comment on lines
91
to
+99
There was a problem hiding this comment. When a JS config file exists, the code still attempts to inject storageCapabilities and version from the backend, but it doesn't inject the backend's uiConfig. This creates an inconsistency: with JS config, developers lose access to the backend's uiConfig as a baseline, which contradicts one of the key benefits mentioned in the PR description ("automatic backend sync"). Consider whether JS config should also have access to backend uiConfig values, perhaps by making them available as a parameter or global variable that the JS config function can reference if needed. |
||||||
| } catch (err) { | ||||||
| console.error('[jaeger-ui-config] Error loading jaeger-ui.config.js:', err); | ||||||
|
Comment on lines
100
to
101
There was a problem hiding this comment. The error object 'err' is declared but never used in this catch block. The error should either be logged for debugging purposes (especially helpful during development) or the parameter should be removed if intentionally unused. Consider changing to 'console.error' or adding a comment explaining why the error is deliberately ignored. |
||||||
| } | ||||||
|
Comment on lines
91
to
102
There was a problem hiding this comment. When a jaeger-ui.config.js file exists, the code doesn't inject the finalUiConfig into the HTML because it's inside the else block (lines 101-135). This means that even if the backend provides uiConfig, it won't be injected when using the JS config file. The storageCapabilities and version will be injected (lines 137-151), but JAEGER_CONFIG won't be set, potentially breaking the UI. The logic should inject JAEGER_CONFIG regardless of which config path is taken, or document why this is intentional. |
||||||
| } else { | ||||||
| // Handle JSON config merged on top of backend UI config | ||||||
| let finalUiConfig = backendUiConfig; | ||||||
|
|
||||||
| if (fs.existsSync(jsonConfigPath)) { | ||||||
| try { | ||||||
| const jsonContent = fs.readFileSync(jsonConfigPath, 'utf-8'); | ||||||
| const parsedJsonConfig = JSON.parse(jsonContent); | ||||||
|
|
||||||
| // Shallow merge: JSON on top of backend base | ||||||
| finalUiConfig = { ...backendUiConfig, ...parsedJsonConfig }; | ||||||
|
There was a problem hiding this comment. The shallow merge approach here differs from how the get-config.ts handles merging. In get-config.ts (lines 42-47), only fields listed in mergeFields are shallow merged, while other fields are completely overwritten. However, this implementation always does a shallow merge of the entire uiConfig object regardless of field semantics. This could lead to unexpected behavior where nested properties from backendUiConfig inadvertently bleed through when they should be completely replaced. Consider applying the same mergeFields logic here as used in get-config.ts to maintain consistency. |
||||||
|
|
||||||
| mergeFields.forEach(key => { | ||||||
| if ( | ||||||
| parsedJsonConfig && | ||||||
| typeof parsedJsonConfig[key] === 'object' && | ||||||
| parsedJsonConfig[key] !== null | ||||||
| ) { | ||||||
| const backendValue = backendUiConfig ? backendUiConfig[key] : {}; | ||||||
| finalUiConfig[key] = { ...backendValue, ...parsedJsonConfig[key] }; | ||||||
| } | ||||||
| }); | ||||||
|
|
||||||
| console.log( | ||||||
| '[jaeger-ui-config] Merged config from jaeger-ui.config.json on top of backend uiConfig' | ||||||
| ); | ||||||
| } catch (err) { | ||||||
| console.error('[jaeger-ui-config] Error loading jaeger-ui.config.json:', err); | ||||||
|
Comment on lines
+129
to
+130
There was a problem hiding this comment. The error object 'err' is declared but never used in this catch block. Similar to the issue above, the error should either be logged for debugging or the parameter should be removed. This is especially important here since JSON parsing errors would be helpful to see during development. |
||||||
| } | ||||||
| } else if (backendUiConfig) { | ||||||
| console.log('[jaeger-ui-config] Using backend uiConfig as base'); | ||||||
| } | ||||||
|
|
||||||
| // Inject final UI config into index.html | ||||||
| if (finalUiConfig) { | ||||||
| html = html.replace( | ||||||
| 'const JAEGER_CONFIG = DEFAULT_CONFIG;', | ||||||
| `const JAEGER_CONFIG = ${JSON.stringify(finalUiConfig)};` | ||||||
| ); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| // Inject storageCapabilities into index.html | ||||||
| if (storageCapabilities) { | ||||||
| html = html.replace( | ||||||
| 'const JAEGER_STORAGE_CAPABILITIES = DEFAULT_STORAGE_CAPABILITIES;', | ||||||
| `const JAEGER_STORAGE_CAPABILITIES = ${JSON.stringify(storageCapabilities)};` | ||||||
| ); | ||||||
| } | ||||||
|
|
||||||
| // Inject version into index.html | ||||||
| if (version) { | ||||||
| html = html.replace( | ||||||
| 'const JAEGER_VERSION = DEFAULT_VERSION;', | ||||||
| `const JAEGER_VERSION = ${JSON.stringify(version)};` | ||||||
|
Comment on lines
+140
to
+157
There was a problem hiding this comment. The JSON.stringify calls on lines 129, 138, and 146 inject configuration data into a script tag but don't escape the string |
||||||
| ); | ||||||
| } | ||||||
|
Comment on lines
+137
to
+159
There was a problem hiding this comment. Multiple JSON.stringify calls (lines 131, 140, 148) are used on data fetched from the backend without error handling. If the backend returns objects with circular references or other non-serializable content, these calls will throw and crash the dev server during HTML transformation. Consider wrapping these operations in try-catch blocks or at minimum documenting the assumption that backend data is always JSON-serializable. |
||||||
|
|
||||||
| return html; | ||||||
| }, | ||||||
| }, | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,5 @@ | ||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||
| * This file previously contained a standalone OpenTelemetry verification script. | ||||||||||||||||||||||||||||||||||||
| * It has been intentionally left empty to avoid keeping unreferenced tooling | ||||||||||||||||||||||||||||||||||||
| * at the repository root. | ||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||
|
Comment on lines
+2
to
+5
There was a problem hiding this comment. This file has been intentionally emptied but not removed. If the file is no longer needed and there are no references to it, consider deleting it entirely rather than leaving an empty file with a comment. An empty file in the repository root can be confusing for future contributors who might wonder if it serves a purpose.
Suggested change
|
||||||||||||||||||||||||||||||||||||
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.
The repo declares
engines.nodeas>=24, but this adds@types/node^25.2.3. If the project is targeting Node 24 in CI/dev, consider aligning the major version of@types/nodewith the supported runtime (or pinning a known-good version) to avoid type drift and incompatibilities.