feat(log-explorer): support ExplorerNG for VictoriaLogs and Loki#2165
Open
hujter wants to merge 14 commits into
Open
feat(log-explorer): support ExplorerNG for VictoriaLogs and Loki#2165hujter wants to merge 14 commits into
hujter wants to merge 14 commits into
Conversation
Add Loki LogQL completion support and query input commit semantics. Reuse resizable query input behavior for VictoriaLogs, sync raw log limit with builder, cap Loki raw log limit at 5000, and format builder-generated queries for multiline editing.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds Loki and VictoriaLogs ExplorerNG plugins with query builders, raw and metric views, API services, query parsing/rendering utilities, context and highlighting support, localization, routing, URL/form integration, and shared log explorer configuration. ChangesShared Log Explorer Integration
Loki ExplorerNG Plugin
VictoriaLogs ExplorerNG Plugin
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (10)
src/plugins/victorialogs/ExplorerNG/utils/logsQL.test.ts-51-66 (1)
51-66: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
VictoriaLogsOrderByhas noidfield — test data triggers TypeScript excess property error.The
orderByitem{ id: '3', field: 'count', direction: 'desc' }includes anidproperty that doesn't exist onVictoriaLogsOrderBy(which only hasfieldanddirection). This would fail TypeScript's excess property checking. Either addidtoVictoriaLogsOrderByfor consistency withVictoriaLogsFilterandVictoriaLogsAggregation, or removeidfrom the test data.💚 Proposed fix (remove id from test data)
- orderBy: [{ id: '3', field: 'count', direction: 'desc' }], + orderBy: [{ field: 'count', direction: 'desc' }],🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plugins/victorialogs/ExplorerNG/utils/logsQL.test.ts` around lines 51 - 66, The multiline metric LogsQL test is using an invalid `orderBy` shape because `VictoriaLogsOrderBy` does not define an `id` field. Update the `renderMetricLogsQL` test case in `logsQL.test.ts` to remove the extra `id` from the `orderBy` fixture, or alternatively align the `VictoriaLogsOrderBy` type with the other builder models if `id` is meant to be supported. Keep the test focused on `renderMetricLogsQL` and `classifyExplorerMode` behavior while using a type-correct `orderBy` item.src/plugins/victorialogs/ExplorerNG/utils/logsQL.ts-115-118 (1)
115-118: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse LogsQL’s quoted wildcard form for
contains/not_contains.escapeValue("hello world")currently yieldsfield:*"hello world"*, but LogsQL expects the wildcard inside the quotes, e.g.field:"*hello world*"for values with spaces or special characters.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plugins/victorialogs/ExplorerNG/utils/logsQL.ts` around lines 115 - 118, The LogsQL query builder in logsQL.ts handles contains/not_contains with the wildcard outside the quoted value, which produces invalid quoted searches for values with spaces or special characters. Update the logic in the query construction branch for the `contains` and `not_contains` cases so the wildcard is placed inside the quoted string while still using `escapeValue(value)`; keep the behavior centralized in this formatter so all callers get the correct LogsQL form.src/plugins/loki/ExplorerNG/Builder/index.tsx-118-127 (1)
118-127: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse theme variable instead of hardcoded
#ff4d4f.The
RequiredLabelcomponent hardcodes#ff4d4ffor the required asterisk. This should use a color/theme variable fromsrc/theme/variable.cssto maintain consistency with the theme system and support dark mode. As per coding guidelines, avoid magic color values and use theme-related values from the existing theme system.🎨 Proposed fix
function RequiredLabel(props: { children: React.ReactNode }) { return ( <span className='relative inline-flex items-center'> - <span style={{ color: '`#ff4d4f`' }} className='absolute right-full mr-1'> + <span style={{ color: 'var(--ant-error-color, `#ff4d4f`)' }} className='absolute right-full mr-1'> * </span> {props.children} </span> ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plugins/loki/ExplorerNG/Builder/index.tsx` around lines 118 - 127, The RequiredLabel component hardcodes the required asterisk color, so replace the inline `#ff4d4f` in `RequiredLabel` with an existing theme variable from the theme system used by the ExplorerNG builder. Update the styling in `Builder/index.tsx` to reference the color token from `src/theme/variable.css` so the asterisk follows app theming and dark mode. Keep the component structure the same and only swap the magic color value for the appropriate theme-based value.Source: Coding guidelines
src/plugins/victorialogs/locale/zh_CN.ts-69-70 (1)
69-70: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
directionandorder_byshare the same Chinese translation.
order_by: '排序'(line 69) anddirection: '排序'(line 70) are identical. In English, "Sort" and "Direction" are distinct —directionrefers to ascending/descending order. Using the same word for both is ambiguous for users.💚 Proposed fix
order_by: '排序', - direction: '排序', + direction: '方向',🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plugins/victorialogs/locale/zh_CN.ts` around lines 69 - 70, The zh_CN localization entries for order_by and direction in the Victorialogs locale currently use the same translation, which makes the UI ambiguous. Update the direction key in the zh_CN file to a Chinese phrase that specifically means ascending/descending order, and keep order_by as the sorting label; use the existing locale keys in the Victorialogs translation map to locate and adjust the text.src/plugins/victorialogs/locale/zh_HK.ts-69-70 (1)
69-70: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
directionandorder_byshare the same Chinese translation.Same issue as
zh_CN.ts:order_by: '排序'anddirection: '排序'are identical.directionrefers to ascending/descending and should be distinguished fromorder_by.💚 Proposed fix
order_by: '排序', - direction: '排序', + direction: '方向',🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plugins/victorialogs/locale/zh_HK.ts` around lines 69 - 70, The zh_HK locale currently uses the same Chinese string for both order_by and direction, so update the direction entry in the locale object to a distinct translation that reflects ascending/descending rather than sorting order. Keep order_by unchanged and adjust the translation in the zh_HK.ts locale definition where these keys are declared.src/plugins/loki/ExplorerNG/utils/optionsLocalstorage.ts-22-24 (1)
22-24: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAdd error handling to
setOptionsToLocalstoragefor consistency withgetOptionsFromLocalstorage.
localStorage.setItemcan throwQuotaExceededErroror fail in private browsing mode. Thegetfunction already has a try-catch forJSON.parse, butsethas none — an uncaught throw here would propagate to the caller (Metric/index.tsxupdateOptions) and crash the UI event handler.🛡️ Proposed fix
export function setOptionsToLocalstorage(logsOptionsCacheKey: string, options: OptionsType) { - localStorage.setItem(`${logsOptionsCacheKey}`@options``, JSON.stringify(options)); + try { + localStorage.setItem(`${logsOptionsCacheKey}`@options``, JSON.stringify(options)); + } catch (e) { + // Silently fail — options won't be persisted + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plugins/loki/ExplorerNG/utils/optionsLocalstorage.ts` around lines 22 - 24, `setOptionsToLocalstorage` currently writes to `localStorage` without any protection, so a quota/private-browsing failure can bubble into `Metric/index.tsx` via `updateOptions` and break the UI handler. Update `setOptionsToLocalstorage` in `optionsLocalstorage.ts` to wrap the `localStorage.setItem` call in a try-catch, mirroring the defensive handling used by `getOptionsFromLocalstorage`, and handle the failure gracefully without throwing to the caller.src/plugins/victorialogs/ExplorerNG/Builder/index.tsx-34-34 (1)
34-34: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAvoid magic color value; use theme variable instead.
#ff4d4fis a hardcoded color that won't adapt to dark mode or theme changes. Use a CSS variable from the theme system (e.g.,var(--ant-error-color)or an equivalent error-color token fromsrc/theme/variable.css).🎨 Proposed fix
- <span style={{ color: '`#ff4d4f`' }} className='absolute right-full mr-1'> + <span style={{ color: 'var(--ant-error-color)' }} className='absolute right-full mr-1'>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plugins/victorialogs/ExplorerNG/Builder/index.tsx` at line 34, The span in Builder/index.tsx uses a hardcoded error red, which should be replaced with a theme-aware color token so it adapts to theme and dark mode changes. Update the styling for the affected span in the ExplorerNG Builder component to use the existing error color variable from the theme system (for example the error token defined in src/theme/variable.css) instead of the literal color value, keeping the same visual intent while avoiding magic values.Source: Coding guidelines
src/plugins/loki/ExplorerNG/Main/Raw/components/ContextViewer.tsx-431-451 (1)
431-451: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winMissing timeout cleanup in scroll effect.
The
window.setTimeouthas no cleanup. Ifdata?.flagorlogchanges before the 100ms fires, a stale timeout can scroll to the wrong position. Add a cleanup return.🔒 Proposed fix
useEffect(() => { const container = tableContainerRef.current; if (!container) return; - window.setTimeout(() => { + const timeoutId = window.setTimeout(() => { if (scrollTarget.current === 'top') { container.scrollTo({ top: 0 }); return; } if (scrollTarget.current === 'bottom') { container.scrollTo({ top: container.scrollHeight }); return; } const currentIdentity = getLogIdentity(log); const row = _.find(Array.from(container.querySelectorAll('tr[data-row-key]')), (item) => item.getAttribute('data-row-key') === currentIdentity) as HTMLElement | undefined; if (row) { container.scrollTo({ top: Math.max(0, row.offsetTop - (container.clientHeight - row.clientHeight) / 2), }); } }, 100); + return () => window.clearTimeout(timeoutId); }, [data?.flag, log]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plugins/loki/ExplorerNG/Main/Raw/components/ContextViewer.tsx` around lines 431 - 451, The scroll effect in ContextViewer’s useEffect schedules a window.setTimeout without cleanup, which can leave stale timers that fire after data?.flag or log changes. Update the effect to store the timeout id and return a cleanup function that clears it, keeping the existing scrollTarget/getLogIdentity logic intact so only the latest effect instance can scroll the tableContainerRef container.src/plugins/loki/ExplorerNG/Main/Raw/components/ContextViewer.tsx-294-305 (1)
294-305: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMagic color values in highlight inline styles.
#f50andyelloware hardcoded in HTML string inline styles. Per coding guidelines, color values should come from the theme system. Consider using CSS classes orvar(--variable)references instead.🎨 Proposed fix using CSS classes
- html = html.replace(filterPattern, "<span style='color: `#f50`;'>$1</span>"); + html = html.replace(filterPattern, "<span class='loki-context-filter-highlight'>$1</span>"); } const bgPattern = highlightPattern(highlightKeywords); if (bgPattern) { - html = html.replace(bgPattern, "<span style='background-color: yellow;'>$1</span>"); + html = html.replace(bgPattern, "<span class='loki-context-bg-highlight'>$1</span>"); }Then add the classes to
style.less:.loki-context-filter-highlight { color: var(--error-color, `#f50`); } .loki-context-bg-highlight { background-color: var(--warning-color, yellow); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plugins/loki/ExplorerNG/Main/Raw/components/ContextViewer.tsx` around lines 294 - 305, The inline highlight styles in renderMarkedText use hardcoded color values, so replace the string-based style injections with theme-driven styling. Update ContextViewer’s renderMarkedText to use CSS classes or theme variables instead of the `#f50` and yellow values, and add matching styles for the highlight spans in the component stylesheet so the filter/highlight rendering still works through the theme system.Source: Coding guidelines
src/plugins/loki/ExplorerNG/Main/Metric/ResetZoomButton.tsx-13-33 (1)
13-33: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winHardcoded "Reset zoom" text not localized.
The parent
Metriccomponent usesuseTranslationfor all user-facing strings. This button text should follow the same pattern.🌍 Proposed fix
import React from 'react'; -import { Button } from 'antd'; +import { Button } from 'antd'; +import { useTranslation } from 'react-i18next'; +import { NAME_SPACE } from '../../../constants'; import uplot from 'uplot'; import classNames from 'classnames'; @@ -13,8 +15,10 @@ export default function ResetZoomButton(props: Props) { const { showResetZoomBtn, getUplot, xScaleInitMinMax, yScaleInitMinMax } = props; + const { t } = useTranslation(NAME_SPACE); return ( <Button @@ -29,7 +33,7 @@ } }} > - Reset zoom + {t('explorer.reset_zoom')} </Button> ); }Note: You'll need to add the
explorer.reset_zoomkey to the locale files.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plugins/loki/ExplorerNG/Main/Metric/ResetZoomButton.tsx` around lines 13 - 33, The ResetZoomButton component still renders a hardcoded user-facing label, while the surrounding Metric UI uses translation hooks for localization. Update ResetZoomButton to use the same translation pattern as Metric by wiring in the translation lookup for the button text, and add the corresponding explorer.reset_zoom key to the locale files so the label is localized consistently.
🧹 Nitpick comments (12)
src/plugins/victorialogs/ExplorerNG/utils/filteredFields.ts (1)
12-14: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winHoist combined builtin fields to module scope to avoid repeated allocation.
[...BUILTIN_FIELDS, ...VICTORIALOGS_BUILTIN_FIELDS]is reconstructed on every iteration of_.filter, allocating a new array for each element. Move it to a module-levelSetfor O(1) lookup and zero per-call allocation.♻️ Proposed refactor
+const ALL_BUILTIN_FIELDS = new Set([...BUILTIN_FIELDS, ...VICTORIALOGS_BUILTIN_FIELDS]); + export function filterOutBuiltinFields(fields: string[]) { - return _.filter(fields, (field) => !_.includes([...BUILTIN_FIELDS, ...VICTORIALOGS_BUILTIN_FIELDS], field)); + return _.filter(fields, (field) => !ALL_BUILTIN_FIELDS.has(field)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plugins/victorialogs/ExplorerNG/utils/filteredFields.ts` around lines 12 - 14, The filterOutBuiltinFields helper is rebuilding the combined builtin fields array inside the _.filter callback on every element, causing repeated allocation and linear lookups. Hoist the merged BUILTIN_FIELDS and VICTORIALOGS_BUILTIN_FIELDS into a module-scope Set in filteredFields.ts, then have filterOutBuiltinFields use that shared Set for O(1) membership checks instead of recreating the array each time.src/plugins/victorialogs/ExplorerNG/types.ts (1)
56-56: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace
range?: anywith a concrete type.Using
anybypasses type safety. Ifrangerepresents a time range specification (e.g.,{ start: number; end: number }or a string alias), declare it explicitly to catch mismatches at compile time.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plugins/victorialogs/ExplorerNG/types.ts` at line 56, The ExplorerNG types currently use range?: any, which bypasses type safety. Update the relevant type definition in types.ts to use a concrete range shape or union (for example, a structured start/end object or a string alias) so the ExplorerNG APIs catch invalid values at compile time. Locate the range field in the ExplorerNG type definitions and replace any with the explicit type used by the rest of the plugin.src/plugins/loki/ExplorerNG/Main/LogQLInput.tsx (2)
37-48: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove unused
onContentChangeprop.
onContentChangeis declared inLokiLogQLInputPropsbut never destructured or used in the component. This is dead code that could confuse consumers of the API.♻️ Proposed fix
export interface LokiLogQLInputProps { value?: string; datasourceValue?: number; range?: any; readonly?: boolean; placeholder?: string; onChange?: (value?: string) => void; onExecute?: (value?: string) => void; - onContentChange?: () => void; onFocus?: () => void; onBlur?: () => void; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plugins/loki/ExplorerNG/Main/LogQLInput.tsx` around lines 37 - 48, `onContentChange` is declared in `LokiLogQLInputProps` but never used anywhere in `LogQLInput`; remove this dead prop from the props interface and any related references so the component API matches actual usage. Keep the change focused on `LokiLogQLInputProps` and the `LogQLInput` component signature if it destructures props.
95-97: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse
value || ''for the initial CodeMirror document.
valueis typed asstring | undefined, andEditorState.create({ doc })expects a string. The value sync effect at line 183 already usesvalue || '', but the initial creation at line 96 passesvaluedirectly. Ifundefinedis ever passed, the editor could initialize with unexpected content.🛡️ Proposed fix
const startState = EditorState.create({ - doc: value, + doc: value || '', extensions: [🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plugins/loki/ExplorerNG/Main/LogQLInput.tsx` around lines 95 - 97, The initial CodeMirror state in LogQLInput’s EditorState.create call is passing value directly even though it can be undefined; update the startState doc initialization to use value || '' just like the existing sync effect, so the editor always starts with a string. Refer to the LogQLInput component and its startState / EditorState.create setup when making the change.src/plugins/loki/ExplorerNG/Main/index.tsx (1)
104-118: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider fixing
onLableClicktypo toonLabelClick.The prop name
onLableClickis a misspelling ofonLabelClick. It's consistent withQueryInput.tsxso it works, but should be corrected for clarity.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plugins/loki/ExplorerNG/Main/index.tsx` around lines 104 - 118, The QueryInput prop name is misspelled as onLableClick in the ExplorerNG/Main usage, while the intended name is onLabelClick. Update the prop in the QueryInput call site and make sure the corresponding prop declaration and usage in QueryInput stay aligned so the API is consistently named.src/plugins/victorialogs/ExplorerNG/utils/tableElementMethods.ts (1)
1-16: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract shared selector logic to reduce duplication.
Both
getIsAtBottomandscrollToTopduplicate thequerySelector+ fallback pattern. Extracting a small helper would improve maintainability if additional table-scroll utilities are added later.♻️ Optional refactor
+function getTableElement(antdTableSelector: string, rgdTableSelector: string): HTMLElement | null { + return document.querySelector<HTMLElement>(antdTableSelector) || document.querySelector<HTMLElement>(rgdTableSelector); +} + export function getIsAtBottom(antdTableSelector: string, rgdTableSelector: string) { - const antdTableBody = document.querySelector(antdTableSelector); - const rgdTable = document.querySelector(rgdTableSelector); - const target = antdTableBody || rgdTable; + const target = getTableElement(antdTableSelector, rgdTableSelector); if (!target) return false; return target.scrollTop + target.clientHeight >= target.scrollHeight - 10; } export function scrollToTop(antdTableSelector: string, rgdTableSelector: string) { - const antdTableBody = document.querySelector(antdTableSelector); - const rgdTable = document.querySelector(rgdTableSelector); - const target = antdTableBody || rgdTable; + const target = getTableElement(antdTableSelector, rgdTableSelector); if (target) { target.scrollTop = 0; } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plugins/victorialogs/ExplorerNG/utils/tableElementMethods.ts` around lines 1 - 16, Both getIsAtBottom and scrollToTop repeat the same antdTableSelector/rgdTableSelector querySelector fallback logic. Extract that shared lookup into a small helper in tableElementMethods.ts, then have both functions call it so the target resolution lives in one place and future table-scroll utilities can reuse it.src/plugins/loki/ExplorerNG/services.ts (1)
90-104: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
getParsedFieldsaccesses properties on potentially string items.When
itemis a string (line 93),item.inferred_typeanditem.valuesare accessed on a string primitive. While JS returnsundefined(no crash), this is semantically incorrect. Consider narrowing before property access.♻️ Suggested refactor for type-safe property access
_.map(res.dat || [], (item) => { - const field = _.isString(item) ? item : item.field || item.name; - return { - field, - inferred_type: item.inferred_type, - values: _.map(item.values || [], (value) => _.toString(value)), - }; + if (_.isString(item)) { + return { field: item, inferred_type: undefined, values: [] }; + } + return { + field: item.field || item.name, + inferred_type: item.inferred_type, + values: _.map(item.values || [], (value) => _.toString(value)), + }; }),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plugins/loki/ExplorerNG/services.ts` around lines 90 - 104, The getParsedFields mapping in services.ts reads inferred_type and values from item even when item is a string, so narrow the type before accessing object properties. Update the _.map callback to branch on the result of _.isString(item) (or use a typed guard) and only read item.inferred_type and item.values for non-string items, while keeping the field extraction and filtering/sorting behavior in getParsedFields unchanged.src/plugins/loki/ExplorerNG/utils/logqlCompletion.ts (2)
41-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace
anytypes with proper type annotations.
LokiCompletionSourceParams.rangeandgetRangeMs's parameter are typed asany, which violates the project guideline to avoidany. TheparseRangereturn type from@/components/TimeRangePickershould be used instead.♻️ Proposed type improvements
export interface LokiCompletionSourceParams { datasourceValue?: number; - range?: any; + range?: Parameters<typeof parseRange>[0]; } -function getRangeMs(range: any) { +function getRangeMs(range: LokiCompletionSourceParams['range']) { if (!range) return undefined; const parsed = parseRange(range); return { start: moment(parsed.start).valueOf(), end: moment(parsed.end).valueOf(), }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plugins/loki/ExplorerNG/utils/logqlCompletion.ts` around lines 41 - 53, Replace the loose `any` usage in `LokiCompletionSourceParams.range` and `getRangeMs` with the proper time range type from `@/components/TimeRangePicker`. Update the `LokiCompletionSourceParams` interface and the `getRangeMs` signature to use that shared type (or the exact `parseRange` input/output type if applicable), so the completion logic remains type-safe and consistent with the project’s no-`any` rule.Source: Coding guidelines
81-87: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse concrete types instead of
any[]for label field/value mappers.
labelFieldsToCompletionsandlabelValuesToCompletionsacceptany[]but the services returnFieldNameSuggestion[]andFieldValueSuggestion[]respectively. Using the concrete types improves type safety and IDE autocompletion.♻️ Proposed type fixes
+import { FieldNameSuggestion, FieldValueSuggestion } from '../services'; + -function labelFieldsToCompletions(fields: any[]) { +function labelFieldsToCompletions(fields: FieldNameSuggestion[]) { return fields.map((item) => ({ label: item.field, type: 'variable', detail: 'label' })); } -function labelValuesToCompletions(values: any[], label: string) { +function labelValuesToCompletions(values: FieldValueSuggestion[], label: string) { return values.map((item) => ({ label: item.value, apply: item.value, type: 'constant', detail: label })); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plugins/loki/ExplorerNG/utils/logqlCompletion.ts` around lines 81 - 87, The mappers in logqlCompletion.ts are still typed with any[], which loses the service contracts and weakens type safety; update labelFieldsToCompletions and labelValuesToCompletions to use the concrete suggestion types returned by the services, namely FieldNameSuggestion for fields and FieldValueSuggestion for values. Make sure the function signatures and any related imports or type references in logqlCompletion.ts align with those symbols so the item.field and item.value accesses remain strongly typed.Source: Coding guidelines
src/plugins/loki/ExplorerNG/utils/renderBuiltinFields.tsx (1)
7-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider replacing
anywithunknownfor stricter type safety.The coding guidelines for
src/**/*.{ts,tsx}state to avoidany. Four function signatures here useanyfor value parameters. Since log values are inherently dynamic,unknownwith appropriate type narrowing would maintain flexibility while improving type safety.As per coding guidelines: "Component Props must use
interfacefor explicit declaration; avoidany".♻️ Optional refactor: replace `any` with `unknown`
-function stringifyValue(value: any) { +function stringifyValue(value: unknown) { if (_.isPlainObject(value) || _.isArray(value)) { return JSON.stringify(value); } return _.toString(value ?? ''); } -function formatTimestamp(value: any) { +function formatTimestamp(value: unknown) { - const text = _.toString(value); + const text = _.toString(value); if (!text) return ''; if (/^\d+$/.test(text)) { const timestamp = text.length >= 13 ? Number(text.slice(0, 13)) : Number(text) * 1000; if (Number.isFinite(timestamp)) { return moment(timestamp).format('YYYY-MM-DD HH:mm:ss.SSS'); } } - const parsed = moment(value); + const parsed = moment(value as moment.MomentInput); return parsed.isValid() ? parsed.format('YYYY-MM-DD HH:mm:ss.SSS') : text; } -function formatValue(field: string, value: any) { +function formatValue(field: string, value: unknown) { if (field === '__timestamp__') { return formatTimestamp(value); } return stringifyValue(value); } -export default function renderBuiltinFields(log: Record<string, any>) { +export default function renderBuiltinFields(log: Record<string, unknown>) {Also applies to: 36-36
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plugins/loki/ExplorerNG/utils/renderBuiltinFields.tsx` around lines 7 - 34, The helper functions in renderBuiltinFields.tsx currently use any for dynamic log values, which conflicts with the stricter typing guideline. Update the value parameters in stringifyValue, formatTimestamp, and formatValue to unknown, then add the necessary type narrowing or guards before calling JSON.stringify, _.toString, or moment. Keep the existing behavior intact while making the types explicit and safe.Source: Coding guidelines
src/plugins/victorialogs/ExplorerNG/Main/index.tsx (1)
140-182: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
onPreviewQLandonExecuteshare duplicated form-update logic.Both handlers construct the same
form.setFieldsValuecall with identical builder/query/keys/vizType mapping. Extract a shared helper to reduce duplication and prevent drift if the form shape changes.♻️ Proposed refactor
+ const applyBuilderResult = (query: string, values: { raw?: VictoriaLogsRawBuilderState; metric?: VictoriaLogsMetricBuilderState; vizType?: 'table' | 'timeseries' }) => { + const oldQuery = form.getFieldValue('query') || {}; + const keys = values.metric ? inferMetricTimeseriesKeys(values.metric) : oldQuery.keys; + form.setFieldsValue({ + query: { + ...oldQuery, + query, + builder: { + ...(oldQuery.builder || {}), + raw: values.raw, + metric: values.metric, + }, + builderStatus: 'synced', + querySource: 'builder', + vizType: values.vizType || oldQuery.vizType, + keys, + }, + }); + }; + <Builder ... onPreviewQL={(query, values) => { - const oldQuery = form.getFieldValue('query') || {}; - const keys = values.metric ? inferMetricTimeseriesKeys(values.metric) : oldQuery.keys; - form.setFieldsValue({ - query: { - ...oldQuery, - query, - builder: { - ...(oldQuery.builder || {}), - raw: values.raw, - metric: values.metric, - }, - builderStatus: 'synced', - querySource: 'builder', - vizType: values.vizType || oldQuery.vizType, - keys, - }, - }); + applyBuilderResult(query, values); setIsContentChangedDotVisible(true); setQueryBuilderVisible(false); }} onExecute={(query, values) => { - const oldQuery = form.getFieldValue('query') || {}; - const keys = values.metric ? inferMetricTimeseriesKeys(values.metric) : oldQuery.keys; - form.setFieldsValue({ - query: { - ...oldQuery, - query, - builder: { - ...(oldQuery.builder || {}), - raw: values.raw, - metric: values.metric, - }, - builderStatus: 'synced', - querySource: 'builder', - vizType: values.vizType || oldQuery.vizType, - keys, - }, - }); + applyBuilderResult(query, values); executeQuery(); setIsContentChangedDotVisible(false); setQueryBuilderVisible(false); }} />🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plugins/victorialogs/ExplorerNG/Main/index.tsx` around lines 140 - 182, Both onPreviewQL and onExecute in ExplorerNG/Main/index.tsx duplicate the same query form update logic, so extract the shared form.setFieldsValue mapping into a helper that builds the next query state from oldQuery, query, and values. Reuse that helper in both handlers to keep builder, keys, vizType, querySource, and builderStatus updates consistent and avoid drift if the form shape changes.src/plugins/loki/ExplorerNG/Main/Raw/components/ContextViewer.tsx (1)
42-164: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared logic from
DynamicTagsandDynamicTagsTrigger.Both components share identical state management (
inputVisible,inputValue,inputRef), the focus effect, andconfirmInputlogic. Extract a custom hook to eliminate duplication.♻️ Proposed refactor: extract `useTagInput` hook
function useTagInput(value: string[], onChange?: (tags: string[]) => void) { const [inputVisible, setInputVisible] = useState(false); const [inputValue, setInputValue] = useState(''); const inputRef = useRef<InputRef>(null); useEffect(() => { if (inputVisible) inputRef.current?.focus(); }, [inputVisible]); const confirmInput = () => { if (inputValue && !_.includes(value, inputValue)) { onChange?.([...value, inputValue]); } setInputVisible(false); setInputValue(''); }; return { inputVisible, setInputVisible, inputValue, setInputValue, inputRef, confirmInput }; }Then both
DynamicTagsandDynamicTagsTriggercan consume it, removing ~30 lines of duplicated code.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plugins/loki/ExplorerNG/Main/Raw/components/ContextViewer.tsx` around lines 42 - 164, Both DynamicTags and DynamicTagsTrigger duplicate the same tag-input state, focus effect, and confirmInput behavior. Extract that shared logic into a custom hook such as useTagInput and have both components consume it, reusing the same inputVisible, inputValue, inputRef, and confirmInput handling while keeping DynamicTags-specific edit behavior separate.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/plugins/loki/ExplorerNG/Builder/index.tsx`:
- Around line 137-156: The label suggestion request in useLabelNameSuggestions
is not refreshing on time range changes because refreshDeps uses boolean
coercion for range?.start and range?.end, so different valid ranges collapse to
the same values. Update useLabelNameSuggestions to depend on the actual numeric
start/end values from getRangeMs(params.range), matching the pattern used by
useParsedFieldSuggestions and LabelValueSelect, so changes to the time range
trigger a new request.
In `@src/plugins/loki/ExplorerNG/Main/Metric/index.tsx`:
- Around line 281-289: The tableService .catch block in Metric/index.tsx is
swallowing errors and returning empty data, so add error handling consistent
with the timeseries path and ContextViewer pattern. Update the table load flow
to surface a user-facing error message instead of only returning fallback data,
using the existing message API and a new locale key for the failure text. Keep
loadTimeRef.current reset behavior, and use the tableService/table path in
Metric/index.tsx as the place to log or display the error before returning the
empty result.
In `@src/plugins/loki/ExplorerNG/Main/Raw/index.tsx`:
- Around line 147-156: The catch blocks in Raw/index.tsx for the logs and
histogram requests are swallowing real query failures and returning empty data,
so add user-facing error feedback there instead of silently falling back to an
empty state. Update the existing async handlers around the logs fetch and
histogram fetch to call message.error with a localized failure string, similar
to ContextViewer’s error handling, and keep the empty-data fallback only as the
returned shape. Be sure to import message from antd and use the existing
translation pattern (or add a locale key) so the failure is visible to users
when the request rejects.
In `@src/plugins/loki/ExplorerNG/utils/logsQL.test.ts`:
- Around line 49-65: Add test coverage in logsQL.test.ts for renderMetricLogQL
when vectorAgg is topk or bottomk with groupBy set, similar to the existing
multiline sum-by(app) case. Use the renderMetricLogQL and classifyExplorerMode
assertions to verify the generated LogQL includes the groupClause for the
topk/bottomk branch, so the regression in the vectorAgg handling is caught.
In `@src/plugins/loki/ExplorerNG/utils/logsQL.ts`:
- Around line 86-95: The `buildVectorAggregation` logic in `logsQL.ts` drops
grouping labels for `topk` and `bottomk` because that branch returns before
applying `groupClause`. Update the `vectorAgg === 'topk' || vectorAgg ===
'bottomk'` path to include the same `by`/`without` clause handling used by the
other aggregations, so the rendered query preserves `groupBy` from the Builder
and emits valid LogQL like `topk by (...) (...)` or `bottomk without (...)
(...)`.
In `@src/plugins/loki/locale/ru_RU.ts`:
- Around line 1-64: The ru_RU locale entries are still using English text, so
translate every user-facing string in the ru_RU object to proper Russian. Update
the labels under explorer.timeseries, context, and builder while keeping the
same keys and structure so the locale remains compatible with the existing i18n
lookups in ru_RU.ts.
In `@src/plugins/victorialogs/ExplorerNG/Builder/index.tsx`:
- Around line 303-307: The filterText helper in the Victorialogs ExplorerNG
builder is dropping valid falsy values because it uses item.value || '' before
stringifying. Update filterText to preserve 0 and false by switching to nullish
handling or directly stringifying item.value, while keeping the existing
exists/not_exists branch and opLabel lookup unchanged.
- Line 119: The `refreshDeps` array in `Builder/index.tsx` is using boolean
coercion for `range` and only tracks truthiness, so changes to the actual time
range do not trigger a refetch. Update the dependency list used by the `Builder`
refresh logic to include the real `range?.start` and `range?.end` values,
matching how `useFieldNameSuggestions` is wired, so aggregation field
suggestions refresh whenever the time range changes.
- Around line 632-643: In OrderByPopover’s open/close handling, the form
validation promise is missing a catch and the form is not reset when opening for
a new item, so align it with FilterPopover and AggregationPopover by adding a
catch to the validateFields() chain and resetting the form before applying
incoming data in the v === true branch. Use the existing form, validateFields,
form.resetFields, and form.setFieldsValue flow in the same block so stale values
are cleared and rejected validation does not become unhandled.
In `@src/plugins/victorialogs/ExplorerNG/Main/Raw/index.tsx`:
- Around line 182-200: In the promise failure handler in Raw/index.tsx, the
append flow currently resets the viewer by returning an empty payload in both
the isNoDataError and generic error branches. Update the catch logic around the
append request so that when appendRef.current is true, it preserves the existing
list, total, and fields instead of returning empty values, while still resetting
appendRef.current and handling the error message/loadTimeRef as needed. Use the
existing catch block and isNoDataError path to distinguish append failures from
initial-load failures, and only clear the rows when this is not an append
request.
In `@src/plugins/victorialogs/ExplorerNG/utils/logsQL.ts`:
- Around line 146-152: The renderAggregation helper in logsQL.ts handles
quantile before checking for an empty field, which allows malformed output when
aggregation.field is missing. Update renderAggregation so the empty-field guard
runs before the quantile branch, and ensure quantile only formats a query when a
field is present; otherwise return an empty string like the other invalid cases.
---
Minor comments:
In `@src/plugins/loki/ExplorerNG/Builder/index.tsx`:
- Around line 118-127: The RequiredLabel component hardcodes the required
asterisk color, so replace the inline `#ff4d4f` in `RequiredLabel` with an
existing theme variable from the theme system used by the ExplorerNG builder.
Update the styling in `Builder/index.tsx` to reference the color token from
`src/theme/variable.css` so the asterisk follows app theming and dark mode. Keep
the component structure the same and only swap the magic color value for the
appropriate theme-based value.
In `@src/plugins/loki/ExplorerNG/Main/Metric/ResetZoomButton.tsx`:
- Around line 13-33: The ResetZoomButton component still renders a hardcoded
user-facing label, while the surrounding Metric UI uses translation hooks for
localization. Update ResetZoomButton to use the same translation pattern as
Metric by wiring in the translation lookup for the button text, and add the
corresponding explorer.reset_zoom key to the locale files so the label is
localized consistently.
In `@src/plugins/loki/ExplorerNG/Main/Raw/components/ContextViewer.tsx`:
- Around line 431-451: The scroll effect in ContextViewer’s useEffect schedules
a window.setTimeout without cleanup, which can leave stale timers that fire
after data?.flag or log changes. Update the effect to store the timeout id and
return a cleanup function that clears it, keeping the existing
scrollTarget/getLogIdentity logic intact so only the latest effect instance can
scroll the tableContainerRef container.
- Around line 294-305: The inline highlight styles in renderMarkedText use
hardcoded color values, so replace the string-based style injections with
theme-driven styling. Update ContextViewer’s renderMarkedText to use CSS classes
or theme variables instead of the `#f50` and yellow values, and add matching
styles for the highlight spans in the component stylesheet so the
filter/highlight rendering still works through the theme system.
In `@src/plugins/loki/ExplorerNG/utils/optionsLocalstorage.ts`:
- Around line 22-24: `setOptionsToLocalstorage` currently writes to
`localStorage` without any protection, so a quota/private-browsing failure can
bubble into `Metric/index.tsx` via `updateOptions` and break the UI handler.
Update `setOptionsToLocalstorage` in `optionsLocalstorage.ts` to wrap the
`localStorage.setItem` call in a try-catch, mirroring the defensive handling
used by `getOptionsFromLocalstorage`, and handle the failure gracefully without
throwing to the caller.
In `@src/plugins/victorialogs/ExplorerNG/Builder/index.tsx`:
- Line 34: The span in Builder/index.tsx uses a hardcoded error red, which
should be replaced with a theme-aware color token so it adapts to theme and dark
mode changes. Update the styling for the affected span in the ExplorerNG Builder
component to use the existing error color variable from the theme system (for
example the error token defined in src/theme/variable.css) instead of the
literal color value, keeping the same visual intent while avoiding magic values.
In `@src/plugins/victorialogs/ExplorerNG/utils/logsQL.test.ts`:
- Around line 51-66: The multiline metric LogsQL test is using an invalid
`orderBy` shape because `VictoriaLogsOrderBy` does not define an `id` field.
Update the `renderMetricLogsQL` test case in `logsQL.test.ts` to remove the
extra `id` from the `orderBy` fixture, or alternatively align the
`VictoriaLogsOrderBy` type with the other builder models if `id` is meant to be
supported. Keep the test focused on `renderMetricLogsQL` and
`classifyExplorerMode` behavior while using a type-correct `orderBy` item.
In `@src/plugins/victorialogs/ExplorerNG/utils/logsQL.ts`:
- Around line 115-118: The LogsQL query builder in logsQL.ts handles
contains/not_contains with the wildcard outside the quoted value, which produces
invalid quoted searches for values with spaces or special characters. Update the
logic in the query construction branch for the `contains` and `not_contains`
cases so the wildcard is placed inside the quoted string while still using
`escapeValue(value)`; keep the behavior centralized in this formatter so all
callers get the correct LogsQL form.
In `@src/plugins/victorialogs/locale/zh_CN.ts`:
- Around line 69-70: The zh_CN localization entries for order_by and direction
in the Victorialogs locale currently use the same translation, which makes the
UI ambiguous. Update the direction key in the zh_CN file to a Chinese phrase
that specifically means ascending/descending order, and keep order_by as the
sorting label; use the existing locale keys in the Victorialogs translation map
to locate and adjust the text.
In `@src/plugins/victorialogs/locale/zh_HK.ts`:
- Around line 69-70: The zh_HK locale currently uses the same Chinese string for
both order_by and direction, so update the direction entry in the locale object
to a distinct translation that reflects ascending/descending rather than sorting
order. Keep order_by unchanged and adjust the translation in the zh_HK.ts locale
definition where these keys are declared.
---
Nitpick comments:
In `@src/plugins/loki/ExplorerNG/Main/index.tsx`:
- Around line 104-118: The QueryInput prop name is misspelled as onLableClick in
the ExplorerNG/Main usage, while the intended name is onLabelClick. Update the
prop in the QueryInput call site and make sure the corresponding prop
declaration and usage in QueryInput stay aligned so the API is consistently
named.
In `@src/plugins/loki/ExplorerNG/Main/LogQLInput.tsx`:
- Around line 37-48: `onContentChange` is declared in `LokiLogQLInputProps` but
never used anywhere in `LogQLInput`; remove this dead prop from the props
interface and any related references so the component API matches actual usage.
Keep the change focused on `LokiLogQLInputProps` and the `LogQLInput` component
signature if it destructures props.
- Around line 95-97: The initial CodeMirror state in LogQLInput’s
EditorState.create call is passing value directly even though it can be
undefined; update the startState doc initialization to use value || '' just like
the existing sync effect, so the editor always starts with a string. Refer to
the LogQLInput component and its startState / EditorState.create setup when
making the change.
In `@src/plugins/loki/ExplorerNG/Main/Raw/components/ContextViewer.tsx`:
- Around line 42-164: Both DynamicTags and DynamicTagsTrigger duplicate the same
tag-input state, focus effect, and confirmInput behavior. Extract that shared
logic into a custom hook such as useTagInput and have both components consume
it, reusing the same inputVisible, inputValue, inputRef, and confirmInput
handling while keeping DynamicTags-specific edit behavior separate.
In `@src/plugins/loki/ExplorerNG/services.ts`:
- Around line 90-104: The getParsedFields mapping in services.ts reads
inferred_type and values from item even when item is a string, so narrow the
type before accessing object properties. Update the _.map callback to branch on
the result of _.isString(item) (or use a typed guard) and only read
item.inferred_type and item.values for non-string items, while keeping the field
extraction and filtering/sorting behavior in getParsedFields unchanged.
In `@src/plugins/loki/ExplorerNG/utils/logqlCompletion.ts`:
- Around line 41-53: Replace the loose `any` usage in
`LokiCompletionSourceParams.range` and `getRangeMs` with the proper time range
type from `@/components/TimeRangePicker`. Update the
`LokiCompletionSourceParams` interface and the `getRangeMs` signature to use
that shared type (or the exact `parseRange` input/output type if applicable), so
the completion logic remains type-safe and consistent with the project’s
no-`any` rule.
- Around line 81-87: The mappers in logqlCompletion.ts are still typed with
any[], which loses the service contracts and weakens type safety; update
labelFieldsToCompletions and labelValuesToCompletions to use the concrete
suggestion types returned by the services, namely FieldNameSuggestion for fields
and FieldValueSuggestion for values. Make sure the function signatures and any
related imports or type references in logqlCompletion.ts align with those
symbols so the item.field and item.value accesses remain strongly typed.
In `@src/plugins/loki/ExplorerNG/utils/renderBuiltinFields.tsx`:
- Around line 7-34: The helper functions in renderBuiltinFields.tsx currently
use any for dynamic log values, which conflicts with the stricter typing
guideline. Update the value parameters in stringifyValue, formatTimestamp, and
formatValue to unknown, then add the necessary type narrowing or guards before
calling JSON.stringify, _.toString, or moment. Keep the existing behavior intact
while making the types explicit and safe.
In `@src/plugins/victorialogs/ExplorerNG/Main/index.tsx`:
- Around line 140-182: Both onPreviewQL and onExecute in
ExplorerNG/Main/index.tsx duplicate the same query form update logic, so extract
the shared form.setFieldsValue mapping into a helper that builds the next query
state from oldQuery, query, and values. Reuse that helper in both handlers to
keep builder, keys, vizType, querySource, and builderStatus updates consistent
and avoid drift if the form shape changes.
In `@src/plugins/victorialogs/ExplorerNG/types.ts`:
- Line 56: The ExplorerNG types currently use range?: any, which bypasses type
safety. Update the relevant type definition in types.ts to use a concrete range
shape or union (for example, a structured start/end object or a string alias) so
the ExplorerNG APIs catch invalid values at compile time. Locate the range field
in the ExplorerNG type definitions and replace any with the explicit type used
by the rest of the plugin.
In `@src/plugins/victorialogs/ExplorerNG/utils/filteredFields.ts`:
- Around line 12-14: The filterOutBuiltinFields helper is rebuilding the
combined builtin fields array inside the _.filter callback on every element,
causing repeated allocation and linear lookups. Hoist the merged BUILTIN_FIELDS
and VICTORIALOGS_BUILTIN_FIELDS into a module-scope Set in filteredFields.ts,
then have filterOutBuiltinFields use that shared Set for O(1) membership checks
instead of recreating the array each time.
In `@src/plugins/victorialogs/ExplorerNG/utils/tableElementMethods.ts`:
- Around line 1-16: Both getIsAtBottom and scrollToTop repeat the same
antdTableSelector/rgdTableSelector querySelector fallback logic. Extract that
shared lookup into a small helper in tableElementMethods.ts, then have both
functions call it so the target resolution lives in one place and future
table-scroll utilities can reuse it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 582b3929-cbc5-464a-99c8-c5f2584477f7
📒 Files selected for processing (78)
src/pages/logExplorer/Explorer.tsxsrc/pages/logExplorer/ExplorerContent.tsxsrc/pages/logExplorer/constants.tssrc/pages/logExplorer/locale/en_US.tssrc/pages/logExplorer/locale/ja_JP.tssrc/pages/logExplorer/locale/ru_RU.tssrc/pages/logExplorer/locale/zh_CN.tssrc/pages/logExplorer/locale/zh_HK.tssrc/pages/logExplorer/utils/getFormValuesBySearchParams.tssrc/plugins/loki/ExplorerNG/Builder/index.tsxsrc/plugins/loki/ExplorerNG/Main/LogQLInput.tsxsrc/plugins/loki/ExplorerNG/Main/Metric/ResetZoomButton.tsxsrc/plugins/loki/ExplorerNG/Main/Metric/index.tsxsrc/plugins/loki/ExplorerNG/Main/QueryInput.tsxsrc/plugins/loki/ExplorerNG/Main/Raw/components/ContextViewer.tsxsrc/plugins/loki/ExplorerNG/Main/Raw/index.tsxsrc/plugins/loki/ExplorerNG/Main/Raw/utils/context.test.tssrc/plugins/loki/ExplorerNG/Main/Raw/utils/context.tssrc/plugins/loki/ExplorerNG/Main/Raw/utils/highlights.test.tssrc/plugins/loki/ExplorerNG/Main/Raw/utils/highlights.tssrc/plugins/loki/ExplorerNG/Main/index.tsxsrc/plugins/loki/ExplorerNG/SideBarNav/index.tsxsrc/plugins/loki/ExplorerNG/components/MainMoreOperations.tsxsrc/plugins/loki/ExplorerNG/components/QueryInputAddonAfter.tsxsrc/plugins/loki/ExplorerNG/constants.tssrc/plugins/loki/ExplorerNG/index.tsxsrc/plugins/loki/ExplorerNG/services.tssrc/plugins/loki/ExplorerNG/style.lesssrc/plugins/loki/ExplorerNG/types.tssrc/plugins/loki/ExplorerNG/utils/filteredFields.tssrc/plugins/loki/ExplorerNG/utils/getOperatorsByFieldType.tssrc/plugins/loki/ExplorerNG/utils/logFields.tssrc/plugins/loki/ExplorerNG/utils/logqlCompletion.tssrc/plugins/loki/ExplorerNG/utils/logqlCompletionCache.test.tssrc/plugins/loki/ExplorerNG/utils/logqlCompletionCache.tssrc/plugins/loki/ExplorerNG/utils/logqlCompletionContext.test.tssrc/plugins/loki/ExplorerNG/utils/logqlCompletionContext.tssrc/plugins/loki/ExplorerNG/utils/logsQL.test.tssrc/plugins/loki/ExplorerNG/utils/logsQL.tssrc/plugins/loki/ExplorerNG/utils/optionsLocalstorage.tssrc/plugins/loki/ExplorerNG/utils/renderBuiltinFields.tsxsrc/plugins/loki/ExplorerNG/utils/renderLogViewerFieldValueWithoutFilters.tsxsrc/plugins/loki/constants.tssrc/plugins/loki/index.tsxsrc/plugins/loki/locale/en_US.tssrc/plugins/loki/locale/index.tssrc/plugins/loki/locale/ja_JP.tssrc/plugins/loki/locale/ru_RU.tssrc/plugins/loki/locale/zh_CN.tssrc/plugins/loki/locale/zh_HK.tssrc/plugins/victorialogs/ExplorerNG/Builder/index.tsxsrc/plugins/victorialogs/ExplorerNG/Main/Metric/ResetZoomButton.tsxsrc/plugins/victorialogs/ExplorerNG/Main/Metric/index.tsxsrc/plugins/victorialogs/ExplorerNG/Main/QueryInput.tsxsrc/plugins/victorialogs/ExplorerNG/Main/Raw/index.tsxsrc/plugins/victorialogs/ExplorerNG/Main/index.tsxsrc/plugins/victorialogs/ExplorerNG/SideBarNav/index.tsxsrc/plugins/victorialogs/ExplorerNG/components/MainMoreOperations.tsxsrc/plugins/victorialogs/ExplorerNG/components/QueryInputAddonAfter.tsxsrc/plugins/victorialogs/ExplorerNG/constants.tssrc/plugins/victorialogs/ExplorerNG/index.tsxsrc/plugins/victorialogs/ExplorerNG/services.tssrc/plugins/victorialogs/ExplorerNG/style.lesssrc/plugins/victorialogs/ExplorerNG/types.tssrc/plugins/victorialogs/ExplorerNG/utils/filteredFields.tssrc/plugins/victorialogs/ExplorerNG/utils/getOperatorsByFieldType.tssrc/plugins/victorialogs/ExplorerNG/utils/logsQL.test.tssrc/plugins/victorialogs/ExplorerNG/utils/logsQL.tssrc/plugins/victorialogs/ExplorerNG/utils/optionsLocalstorage.tssrc/plugins/victorialogs/ExplorerNG/utils/renderBuiltinFields.tsxsrc/plugins/victorialogs/ExplorerNG/utils/renderLogViewerFieldValueWithoutFilters.tsxsrc/plugins/victorialogs/ExplorerNG/utils/tableElementMethods.tssrc/plugins/victorialogs/locale/en_US.tssrc/plugins/victorialogs/locale/index.tssrc/plugins/victorialogs/locale/ja_JP.tssrc/plugins/victorialogs/locale/ru_RU.tssrc/plugins/victorialogs/locale/zh_CN.tssrc/plugins/victorialogs/locale/zh_HK.ts
Comment on lines
+137
to
+156
| function useLabelNameSuggestions(params: { datasourceValue?: number; range?: any; query?: string; keyword: string; enabled?: boolean }) { | ||
| const range = useMemo(() => getRangeMs(params.range), [JSON.stringify(params.range)]); | ||
| return useRequest( | ||
| () => | ||
| getLabelNames({ | ||
| cate: DatasourceCateEnum.loki, | ||
| datasource_id: params.datasourceValue!, | ||
| query: normalizeSuggestionQuery(params.query), | ||
| start: range!.start, | ||
| end: range!.end, | ||
| filter: params.keyword, | ||
| limit: 100, | ||
| }), | ||
| { | ||
| refreshDeps: [params.datasourceValue, params.query, params.keyword, !!range?.start, !!range?.end], | ||
| ready: !!params.enabled && !!params.datasourceValue && !!range?.start && !!range?.end, | ||
| debounceWait: SUGGESTION_DEBOUNCE_WAIT, | ||
| }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Label name suggestions won't refresh on time range changes due to boolean coercion in refreshDeps.
useLabelNameSuggestions uses !!range?.start and !!range?.end (boolean) in refreshDeps, while useParsedFieldSuggestions (line 174) and LabelValueSelect (line 244) correctly use range?.start and range?.end (numeric). When the time range changes from one valid range to another, the boolean values remain true, so the request is never refreshed — label suggestions become stale.
🐛 Proposed fix
refreshDeps: [params.datasourceValue, params.query, params.keyword, !!range?.start, !!range?.end],
+ refreshDeps: [params.datasourceValue, params.query, params.keyword, range?.start, range?.end],
ready: !!params.enabled && !!params.datasourceValue && !!range?.start && !!range?.end,📝 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.
Suggested change
| function useLabelNameSuggestions(params: { datasourceValue?: number; range?: any; query?: string; keyword: string; enabled?: boolean }) { | |
| const range = useMemo(() => getRangeMs(params.range), [JSON.stringify(params.range)]); | |
| return useRequest( | |
| () => | |
| getLabelNames({ | |
| cate: DatasourceCateEnum.loki, | |
| datasource_id: params.datasourceValue!, | |
| query: normalizeSuggestionQuery(params.query), | |
| start: range!.start, | |
| end: range!.end, | |
| filter: params.keyword, | |
| limit: 100, | |
| }), | |
| { | |
| refreshDeps: [params.datasourceValue, params.query, params.keyword, !!range?.start, !!range?.end], | |
| ready: !!params.enabled && !!params.datasourceValue && !!range?.start && !!range?.end, | |
| debounceWait: SUGGESTION_DEBOUNCE_WAIT, | |
| }, | |
| ); | |
| } | |
| function useLabelNameSuggestions(params: { datasourceValue?: number; range?: any; query?: string; keyword: string; enabled?: boolean }) { | |
| const range = useMemo(() => getRangeMs(params.range), [JSON.stringify(params.range)]); | |
| return useRequest( | |
| () => | |
| getLabelNames({ | |
| cate: DatasourceCateEnum.loki, | |
| datasource_id: params.datasourceValue!, | |
| query: normalizeSuggestionQuery(params.query), | |
| start: range!.start, | |
| end: range!.end, | |
| filter: params.keyword, | |
| limit: 100, | |
| }), | |
| { | |
| refreshDeps: [params.datasourceValue, params.query, params.keyword, range?.start, range?.end], | |
| ready: !!params.enabled && !!params.datasourceValue && !!range?.start && !!range?.end, | |
| debounceWait: SUGGESTION_DEBOUNCE_WAIT, | |
| }, | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/plugins/loki/ExplorerNG/Builder/index.tsx` around lines 137 - 156, The
label suggestion request in useLabelNameSuggestions is not refreshing on time
range changes because refreshDeps uses boolean coercion for range?.start and
range?.end, so different valid ranges collapse to the same values. Update
useLabelNameSuggestions to depend on the actual numeric start/end values from
getRangeMs(params.range), matching the pattern used by useParsedFieldSuggestions
and LabelValueSelect, so changes to the time range trigger a new request.
Comment on lines
+281
to
+289
| .catch(() => { | ||
| loadTimeRef.current = null; | ||
| return { | ||
| list: [], | ||
| total: 0, | ||
| hash: _.uniqueId('logs_'), | ||
| fields: [], | ||
| }; | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Silent error swallowing in tableService catch block.
The .catch() returns empty data without any error feedback. Users see "no data" instead of an error message. The timeseries path (line 345) at least logs to console.error, but the table path has no logging at all. Add user-facing error feedback consistent with the ContextViewer pattern.
🔧 Proposed fix
.catch(() => {
+ console.error('loki tableService failed');
+ message.error(t(`${logExplorerNS}:logs.query_failed`));
loadTimeRef.current = null;
return {
list: [],
total: 0,
hash: _.uniqueId('logs_'),
fields: [],
};
});Note: You'll need to add message to the antd import on line 4 and add a locale key for the error message.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/plugins/loki/ExplorerNG/Main/Metric/index.tsx` around lines 281 - 289,
The tableService .catch block in Metric/index.tsx is swallowing errors and
returning empty data, so add error handling consistent with the timeseries path
and ContextViewer pattern. Update the table load flow to surface a user-facing
error message instead of only returning fallback data, using the existing
message API and a new locale key for the failure text. Keep loadTimeRef.current
reset behavior, and use the tableService/table path in Metric/index.tsx as the
place to log or display the error before returning the empty result.
Comment on lines
+147
to
+156
| .catch(() => { | ||
| loadTimeRef.current = null; | ||
| return { | ||
| list: [], | ||
| total: 0, | ||
| limit, | ||
| hash: _.uniqueId('logs_'), | ||
| fields: [], | ||
| }; | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Silent error swallowing in logs and histogram catch blocks.
Both .catch() handlers return empty data without any user-facing error feedback. Combined with silence: true in the service layer, users see a "no data" empty state even when the query actually failed, making it impossible to distinguish between "no results" and "query error." The ContextViewer in this same PR correctly calls message.error(t('context.query_failed')) on errors — follow the same pattern here.
🔧 Proposed fix for logs catch (lines 147-156)
.catch(() => {
+ message.error(t(`${logExplorerNS}:logs.query_failed`));
loadTimeRef.current = null;
return {
list: [],
total: 0,
limit,
hash: _.uniqueId('logs_'),
fields: [],
};
});🔧 Proposed fix for histogram catch (lines 203-208)
.catch(() => {
+ message.error(t(`${logExplorerNS}:logs.query_failed`));
return {
data: [],
hash: _.uniqueId('histogram_'),
};
});Note: You'll need to add message to the antd import on line 4 and add a locale key for the error message (or reuse an existing one).
Also applies to: 203-208
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/plugins/loki/ExplorerNG/Main/Raw/index.tsx` around lines 147 - 156, The
catch blocks in Raw/index.tsx for the logs and histogram requests are swallowing
real query failures and returning empty data, so add user-facing error feedback
there instead of silently falling back to an empty state. Update the existing
async handlers around the logs fetch and histogram fetch to call message.error
with a localized failure string, similar to ContextViewer’s error handling, and
keep the empty-data fallback only as the returned shape. Be sure to import
message from antd and use the existing translation pattern (or add a locale key)
so the failure is visible to users when the request rejects.
Comment on lines
+49
to
+65
| it('renders multiline metric LogQL with outer aggregation only', () => { | ||
| const query = renderMetricLogQL( | ||
| { | ||
| labels: [{ id: '1', label: 'app', op: '=', value: 'api' }], | ||
| lineFilters: [{ id: '2', op: '|=', value: 'error' }], | ||
| rangeFunc: 'count_over_time', | ||
| range: '5m', | ||
| vectorAgg: 'sum', | ||
| groupBy: ['app'], | ||
| vizType: 'timeseries', | ||
| }, | ||
| { multiline: true }, | ||
| ); | ||
|
|
||
| expect(query).toBe(['sum by (app) (', ' count_over_time({app="api"} |= "error"[5m])', ')'].join('\n')); | ||
| expect(classifyExplorerMode(query)).toBe('metric'); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add test coverage for topk/bottomk with groupBy.
The current tests cover sum with groupBy but not topk/bottomk with groupBy. Since the topk/bottomk branch in renderMetricLogQL has a missing groupClause (see logsQL.ts review), adding this test would catch the regression and verify the fix.
✅ Suggested test for topk with groupBy
+ it('renders topk with groupBy clause', () => {
+ expect(
+ renderMetricLogQL({
+ labels: [{ id: '1', label: 'app', op: '=', value: 'api' }],
+ rangeFunc: 'count_over_time',
+ range: '5m',
+ vectorAgg: 'topk',
+ vectorParam: 10,
+ groupBy: ['app'],
+ vizType: 'timeseries',
+ }),
+ ).toBe('topk by (app) (10, count_over_time({app="api"}[5m]))');
+ });
+
+ it('renders multiline topk with groupBy clause', () => {
+ const query = renderMetricLogQL(
+ {
+ labels: [{ id: '1', label: 'app', op: '=', value: 'api' }],
+ rangeFunc: 'count_over_time',
+ range: '5m',
+ vectorAgg: 'topk',
+ vectorParam: 5,
+ groupBy: ['app'],
+ vizType: 'timeseries',
+ },
+ { multiline: true },
+ );
+ expect(query).toBe(['topk by (app) (', ' 5, count_over_time({app="api"}[5m])', ')'].join('\n'));
+ });📝 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.
Suggested change
| it('renders multiline metric LogQL with outer aggregation only', () => { | |
| const query = renderMetricLogQL( | |
| { | |
| labels: [{ id: '1', label: 'app', op: '=', value: 'api' }], | |
| lineFilters: [{ id: '2', op: '|=', value: 'error' }], | |
| rangeFunc: 'count_over_time', | |
| range: '5m', | |
| vectorAgg: 'sum', | |
| groupBy: ['app'], | |
| vizType: 'timeseries', | |
| }, | |
| { multiline: true }, | |
| ); | |
| expect(query).toBe(['sum by (app) (', ' count_over_time({app="api"} |= "error"[5m])', ')'].join('\n')); | |
| expect(classifyExplorerMode(query)).toBe('metric'); | |
| }); | |
| it('renders multiline metric LogQL with outer aggregation only', () => { | |
| const query = renderMetricLogQL( | |
| { | |
| labels: [{ id: '1', label: 'app', op: '=', value: 'api' }], | |
| lineFilters: [{ id: '2', op: '|=', value: 'error' }], | |
| rangeFunc: 'count_over_time', | |
| range: '5m', | |
| vectorAgg: 'sum', | |
| groupBy: ['app'], | |
| vizType: 'timeseries', | |
| }, | |
| { multiline: true }, | |
| ); | |
| expect(query).toBe(['sum by (app) (', ' count_over_time({app="api"} |= "error"[5m])', ')'].join('\n')); | |
| expect(classifyExplorerMode(query)).toBe('metric'); | |
| }); | |
| it('renders topk with groupBy clause', () => { | |
| expect( | |
| renderMetricLogQL({ | |
| labels: [{ id: '1', label: 'app', op: '=', value: 'api' }], | |
| rangeFunc: 'count_over_time', | |
| range: '5m', | |
| vectorAgg: 'topk', | |
| vectorParam: 10, | |
| groupBy: ['app'], | |
| vizType: 'timeseries', | |
| }), | |
| ).toBe('topk by (app) (10, count_over_time({app="api"}[5m]))'); | |
| }); | |
| it('renders multiline topk with groupBy clause', () => { | |
| const query = renderMetricLogQL( | |
| { | |
| labels: [{ id: '1', label: 'app', op: '=', value: 'api' }], | |
| rangeFunc: 'count_over_time', | |
| range: '5m', | |
| vectorAgg: 'topk', | |
| vectorParam: 5, | |
| groupBy: ['app'], | |
| vizType: 'timeseries', | |
| }, | |
| { multiline: true }, | |
| ); | |
| expect(query).toBe(['topk by (app) (', ' 5, count_over_time({app="api"}[5m])', ')'].join('\n')); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/plugins/loki/ExplorerNG/utils/logsQL.test.ts` around lines 49 - 65, Add
test coverage in logsQL.test.ts for renderMetricLogQL when vectorAgg is topk or
bottomk with groupBy set, similar to the existing multiline sum-by(app) case.
Use the renderMetricLogQL and classifyExplorerMode assertions to verify the
generated LogQL includes the groupClause for the topk/bottomk branch, so the
regression in the vectorAgg handling is caught.
Comment on lines
+86
to
+95
| if (vectorAgg === 'topk' || vectorAgg === 'bottomk') { | ||
| if (options?.multiline) { | ||
| return `${vectorAgg}(${builder?.vectorParam || 10},\n ${expr}\n)`; | ||
| } | ||
| return `${vectorAgg}(${builder?.vectorParam || 10}, ${expr})`; | ||
| } | ||
| if (options?.multiline) { | ||
| return `${vectorAgg}${groupClause} (\n ${expr}\n)`; | ||
| } | ||
| return `${vectorAgg}${groupClause} (${expr})`; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
topk/bottomk silently drops groupBy labels.
The topk/bottomk branch does not include groupClause, so when a user selects grouping labels with topk/bottomk, those labels are ignored in the rendered query. In LogQL, topk and bottomk support by/without clauses (e.g., topk by (app) (10, count_over_time(...))). The Builder (context snippet 3, line 754) always passes groupBy regardless of vectorAgg, making this a functional bug that produces incorrect query results.
🐛 Proposed fix to include group clause for topk/bottomk
if (vectorAgg === 'topk' || vectorAgg === 'bottomk') {
if (options?.multiline) {
- return `${vectorAgg}(${builder?.vectorParam || 10},\n ${expr}\n)`;
+ return `${vectorAgg}${groupClause} (${builder?.vectorParam || 10},\n ${expr}\n)`;
}
- return `${vectorAgg}(${builder?.vectorParam || 10}, ${expr})`;
+ return `${vectorAgg}${groupClause} (${builder?.vectorParam || 10}, ${expr})`;
}📝 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.
Suggested change
| if (vectorAgg === 'topk' || vectorAgg === 'bottomk') { | |
| if (options?.multiline) { | |
| return `${vectorAgg}(${builder?.vectorParam || 10},\n ${expr}\n)`; | |
| } | |
| return `${vectorAgg}(${builder?.vectorParam || 10}, ${expr})`; | |
| } | |
| if (options?.multiline) { | |
| return `${vectorAgg}${groupClause} (\n ${expr}\n)`; | |
| } | |
| return `${vectorAgg}${groupClause} (${expr})`; | |
| if (vectorAgg === 'topk' || vectorAgg === 'bottomk') { | |
| if (options?.multiline) { | |
| return `${vectorAgg}${groupClause} (${builder?.vectorParam || 10},\n ${expr}\n)`; | |
| } | |
| return `${vectorAgg}${groupClause} (${builder?.vectorParam || 10}, ${expr})`; | |
| } | |
| if (options?.multiline) { | |
| return `${vectorAgg}${groupClause} (\n ${expr}\n)`; | |
| } | |
| return `${vectorAgg}${groupClause} (${expr})`; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/plugins/loki/ExplorerNG/utils/logsQL.ts` around lines 86 - 95, The
`buildVectorAggregation` logic in `logsQL.ts` drops grouping labels for `topk`
and `bottomk` because that branch returns before applying `groupClause`. Update
the `vectorAgg === 'topk' || vectorAgg === 'bottomk'` path to include the same
`by`/`without` clause handling used by the other aggregations, so the rendered
query preserves `groupBy` from the Builder and emits valid LogQL like `topk by
(...) (...)` or `bottomk without (...) (...)`.
| }); | ||
| }, | ||
| { | ||
| refreshDeps: [params.datasourceValue, params.query, !!range?.start, !!range?.end], |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
refreshDeps uses boolean coercion for range values, preventing refetch on time range changes.
!!range?.start and !!range?.end only track whether the values are truthy, not their actual values. When the user changes the time range (e.g., from 1000–2000 to 2000–3000), both booleans stay true and the aggregation field suggestions are never refreshed. Compare with useFieldNameSuggestions (line 97) which correctly uses range?.start, range?.end.
🐛 Proposed fix
- refreshDeps: [params.datasourceValue, params.query, !!range?.start, !!range?.end],
+ refreshDeps: [params.datasourceValue, params.query, range?.start, range?.end],📝 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.
Suggested change
| refreshDeps: [params.datasourceValue, params.query, !!range?.start, !!range?.end], | |
| refreshDeps: [params.datasourceValue, params.query, range?.start, range?.end], |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/plugins/victorialogs/ExplorerNG/Builder/index.tsx` at line 119, The
`refreshDeps` array in `Builder/index.tsx` is using boolean coercion for `range`
and only tracks truthiness, so changes to the actual time range do not trigger a
refetch. Update the dependency list used by the `Builder` refresh logic to
include the real `range?.start` and `range?.end` values, matching how
`useFieldNameSuggestions` is wired, so aggregation field suggestions refresh
whenever the time range changes.
Comment on lines
+303
to
+307
| function filterText(item: VictoriaLogsFilter) { | ||
| const opLabel = _.find(filterOperators, { value: item.op })?.label || item.op; | ||
| if (item.op === 'exists' || item.op === 'not_exists') return opLabel; | ||
| return `${opLabel} ${_.toString(item.value || '')}`; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
filterText discards falsy values 0 and false.
item.value || '' treats 0 and false as falsy, rendering them as empty strings. A filter with value 0 would display as field = instead of field = 0. Use nullish coalescing or direct _.toString to preserve all valid values.
🐛 Proposed fix
function filterText(item: VictoriaLogsFilter) {
const opLabel = _.find(filterOperators, { value: item.op })?.label || item.op;
if (item.op === 'exists' || item.op === 'not_exists') return opLabel;
- return `${opLabel} ${_.toString(item.value || '')}`;
+ return `${opLabel} ${_.toString(item.value ?? '')}`;
}📝 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.
Suggested change
| function filterText(item: VictoriaLogsFilter) { | |
| const opLabel = _.find(filterOperators, { value: item.op })?.label || item.op; | |
| if (item.op === 'exists' || item.op === 'not_exists') return opLabel; | |
| return `${opLabel} ${_.toString(item.value || '')}`; | |
| } | |
| function filterText(item: VictoriaLogsFilter) { | |
| const opLabel = _.find(filterOperators, { value: item.op })?.label || item.op; | |
| if (item.op === 'exists' || item.op === 'not_exists') return opLabel; | |
| return `${opLabel} ${_.toString(item.value ?? '')}`; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/plugins/victorialogs/ExplorerNG/Builder/index.tsx` around lines 303 -
307, The filterText helper in the Victorialogs ExplorerNG builder is dropping
valid falsy values because it uses item.value || '' before stringifying. Update
filterText to preserve 0 and false by switching to nullish handling or directly
stringifying item.value, while keeping the existing exists/not_exists branch and
opLabel lookup unchanged.
Comment on lines
+632
to
+643
| if (v === false) { | ||
| form.validateFields().then((values) => { | ||
| if (data) { | ||
| onChange?.(values); | ||
| } else { | ||
| form.resetFields(); | ||
| onAdd?.(values); | ||
| } | ||
| }); | ||
| } else if (v === true && data) { | ||
| form.setFieldsValue(data); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
OrderByPopover missing .catch() on validateFields() and form.resetFields() on open.
Two issues compared to FilterPopover (lines 354–360) and AggregationPopover (lines 500–506):
- Unhandled promise rejection:
form.validateFields().then(...)has no.catch(). If validation fails, the rejection is unhandled. Add.catch(_.noop)like the other popovers. - Stale form values: When opening without
data(adding new), the form is not reset. Values from a previous edit persist. Theelse ifbranch should callform.resetFields()before conditionally setting data, per the coding guideline thatform.setFieldsValueis incremental and requires a reset for full replacement.
As per coding guidelines: "Note that form.setFieldsValue is an incremental update; unspecified fields retain original values. For full replacement scenarios, reset the form (e.g., form.resetFields()) before setting new values."
🔒 Proposed fix
if (v === false) {
- form.validateFields().then((values) => {
+ form.validateFields().then((values) => {
if (data) {
onChange?.(values);
} else {
form.resetFields();
onAdd?.(values);
}
- });
+ }).catch(_.noop);
} else if (v === true && data) {
- form.setFieldsValue(data);
+ form.resetFields();
+ if (data) {
+ form.setFieldsValue(data);
+ }
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/plugins/victorialogs/ExplorerNG/Builder/index.tsx` around lines 632 -
643, In OrderByPopover’s open/close handling, the form validation promise is
missing a catch and the form is not reset when opening for a new item, so align
it with FilterPopover and AggregationPopover by adding a catch to the
validateFields() chain and resetting the form before applying incoming data in
the v === true branch. Use the existing form, validateFields, form.resetFields,
and form.setFieldsValue flow in the same block so stale values are cleared and
rejected validation does not become unhandled.
Source: Coding guidelines
Comment on lines
+182
to
+200
| .catch((e: any) => { | ||
| appendRef.current = false; | ||
| if (isNoDataError(e)) { | ||
| return { | ||
| list: [], | ||
| total: 0, | ||
| hash: _.uniqueId('logs_'), | ||
| fields: [], | ||
| }; | ||
| } | ||
| message.error(e?.message || e?.msg || 'Query failed'); | ||
| loadTimeRef.current = null; | ||
| return { | ||
| list: [], | ||
| total: 0, | ||
| hash: _.uniqueId('logs_'), | ||
| fields: [], | ||
| }; | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file and inspect the relevant area with line numbers.
ast-grep outline src/plugins/victorialogs/ExplorerNG/Main/Raw/index.tsx --view expanded || true
wc -l src/plugins/victorialogs/ExplorerNG/Main/Raw/index.tsx
sed -n '1,260p' src/plugins/victorialogs/ExplorerNG/Main/Raw/index.tsx | cat -n | sed -n '150,240p'
# Find all references to appendRef, isNoDataError, loadTimeRef, and the request result shape.
rg -n "appendRef|isNoDataError|loadTimeRef|hash: _.uniqueId\('logs_'\)|list:\s*\[\],\s*total:\s*0" src/plugins/victorialogs/ExplorerNG/Main/Raw/index.tsx
# Inspect nearby logic that consumes the request result.
sed -n '240,460p' src/plugins/victorialogs/ExplorerNG/Main/Raw/index.tsx | cat -n | sed -n '1,240p'Repository: n9e/fe
Length of output: 13162
Preserve loaded logs on append failure
When appendRef.current is true, both the no_data and error branches return an empty payload, which clears the already-loaded rows in infinite-scroll mode. Keep the current list, total, and fields for append requests so a transient failure doesn’t wipe the viewer.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/plugins/victorialogs/ExplorerNG/Main/Raw/index.tsx` around lines 182 -
200, In the promise failure handler in Raw/index.tsx, the append flow currently
resets the viewer by returning an empty payload in both the isNoDataError and
generic error branches. Update the catch logic around the append request so that
when appendRef.current is true, it preserves the existing list, total, and
fields instead of returning empty values, while still resetting
appendRef.current and handling the error message/loadTimeRef as needed. Use the
existing catch block and isNoDataError path to distinguish append failures from
initial-load failures, and only clear the rows when this is not an append
request.
Comment on lines
+146
to
+152
| function renderAggregation(aggregation: VictoriaLogsAggregation) { | ||
| const alias = _.trim(aggregation.alias) || aggregation.func; | ||
| if (aggregation.func === 'count') return `count() as ${alias || 'count'}`; | ||
| if (aggregation.func === 'quantile') return `quantile(${aggregation.params?.quantile ?? 0.99}, ${aggregation.field}) as ${alias}`; | ||
| if (!aggregation.field) return ''; | ||
| return `${aggregation.func}(${aggregation.field}) as ${alias}`; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
quantile with empty field produces an invalid query.
The !aggregation.field guard on line 150 is unreachable for quantile because the quantile branch on line 149 returns first. When aggregation.field is empty, this yields quantile(0.99, ) as quantile — a malformed query. Move the field check before the quantile branch.
🐛 Proposed fix
function renderAggregation(aggregation: VictoriaLogsAggregation) {
const alias = _.trim(aggregation.alias) || aggregation.func;
if (aggregation.func === 'count') return `count() as ${alias || 'count'}`;
+ if (!aggregation.field) return '';
if (aggregation.func === 'quantile') return `quantile(${aggregation.params?.quantile ?? 0.99}, ${aggregation.field}) as ${alias}`;
- if (!aggregation.field) return '';
return `${aggregation.func}(${aggregation.field}) as ${alias}`;
}📝 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.
Suggested change
| function renderAggregation(aggregation: VictoriaLogsAggregation) { | |
| const alias = _.trim(aggregation.alias) || aggregation.func; | |
| if (aggregation.func === 'count') return `count() as ${alias || 'count'}`; | |
| if (aggregation.func === 'quantile') return `quantile(${aggregation.params?.quantile ?? 0.99}, ${aggregation.field}) as ${alias}`; | |
| if (!aggregation.field) return ''; | |
| return `${aggregation.func}(${aggregation.field}) as ${alias}`; | |
| } | |
| function renderAggregation(aggregation: VictoriaLogsAggregation) { | |
| const alias = _.trim(aggregation.alias) || aggregation.func; | |
| if (aggregation.func === 'count') return `count() as ${alias || 'count'}`; | |
| if (!aggregation.field) return ''; | |
| if (aggregation.func === 'quantile') return `quantile(${aggregation.params?.quantile ?? 0.99}, ${aggregation.field}) as ${alias}`; | |
| return `${aggregation.func}(${aggregation.field}) as ${alias}`; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/plugins/victorialogs/ExplorerNG/utils/logsQL.ts` around lines 146 - 152,
The renderAggregation helper in logsQL.ts handles quantile before checking for
an empty field, which allows malformed output when aggregation.field is missing.
Update renderAggregation so the empty-field guard runs before the quantile
branch, and ensure quantile only formats a query when a field is present;
otherwise return an empty string like the other invalid cases.
…ogs-loki Co-authored-by: Cursor <cursoragent@cursor.com> # Conflicts: # src/plugins/victorialogs/locale/en_US.ts # src/plugins/victorialogs/locale/ja_JP.ts # src/plugins/victorialogs/locale/ru_RU.ts # src/plugins/victorialogs/locale/zh_CN.ts # src/plugins/victorialogs/locale/zh_HK.ts
…al layout - Remove SideBar/SideBarNav from ExplorerNG, pass indexData as empty array - Change Builder metric sections from conditional render to CSS hidden - Add layout prop to RenderCommonSettingsParams for horizontal support - Improve Main flex layout and add Builder border highlight
…gQL input - Swap CodeMirror editor for LokiMonacoEditor from @fc-components/monaco-editor ^0.5.0 - Remove custom CodeMirror completion logic (logqlCompletion, logqlCompletionContext) - Remove SideBar/SideBarNav wrapper, switch to vertical layout - Remove silence:true from API services so errors surface to users - Convert builder conditional rendering to CSS-based show/hide
… ExplorerNG styling - Replace inline line filter editing with Popover-based LineFilterPopover - Update tags/fields cards styling: border-primary/bg → fc-border/bg-fc-150 - Align text-primary to text-soft and font-medium to font-bold - Unify rounded corners (rounded-sm → rounded-lg) across builders - Add i18n for limit display label in Loki raw view - Remove redundant mt-2 spacing in VictoriaLogs Main
… simplify routing - Set graphPro to false for VictoriaLogs (log datasource, not graph) - Add VictoriaLogs plugin rendering in explorer page - Remove hardcoded datasource exclusion in logExplorer - Rename /log/explorer route to /log/explorer-legacy
…lization - Remove unused SideBarNav component from loki and victorialogs - Add i18n support for query placeholder in LogQLInput - Fix popover placement from bottom to bottomLeft - Strip stale builder state fields on explorer form init - Clean up raw query fallback after RAW_DEFAULT_QUERY change - Bump @fc-components/monaco-editor to 0.5.1
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Summary by CodeRabbit