Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
111 changes: 105 additions & 6 deletions src/platform/packages/shared/kbn-grok-ui/components/expression.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,18 +10,28 @@
import type { CodeEditorProps, monaco } from '@kbn/code-editor';
import { CodeEditor } from '@kbn/code-editor';
import React, { useRef, useState, useEffect, useMemo } from 'react';
import { css } from '@emotion/react';
import { useEuiTheme } from '@elastic/eui';
import { useResizeChecker } from '@kbn/react-hooks';
import { DraftGrokExpression, type GrokCollection } from '../models';
import { colourToClassName } from './utils';

// Matches %{SYNTAX:SEMANTIC} and %{SYNTAX:SEMANTIC:TYPE} tokens
const GROK_FIELD_PATTERN_REGEX =
/%\{[A-Z0-9_@#$%&*+=\-\.]+:([A-Za-z0-9_@#$%&*+=\-\.]+)(?::[A-Za-z]+)?\}/g;

export const Expression = ({
grokCollection,
pattern,
patternSlotId,
onChange,
height = '100px',
dataTestSubj,
}: {
grokCollection: GrokCollection;
pattern: string;
/** Must match the preview draft slot for this row so field colours stay stable while typing. */
patternSlotId?: string | number;
onChange?: (pattern: string) => void;
height?: CodeEditorProps['height'];
dataTestSubj?: string;
Expand All @@ -30,41 +40,75 @@ export const Expression = ({
return grokCollection.getSuggestionProvider();
});

const { euiTheme } = useEuiTheme();

const draftGrokExpression = useMemo(() => {
return new DraftGrokExpression(grokCollection, pattern);
return new DraftGrokExpression(grokCollection, pattern, { patternSlotId });
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [grokCollection]);
}, [grokCollection, patternSlotId]);
Comment on lines 45 to +48
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Missing cleanup for draftGrokExpression – subscription and registration leak.

DraftGrokExpression subscribes to customPatternsChanged$ and registers itself as a field-usage source. Without calling destroy() on unmount or when deps change, these persist indefinitely.

Proposed fix
  const draftGrokExpression = useMemo(() => {
    return new DraftGrokExpression(grokCollection, pattern, { patternSlotId });
    // eslint-disable-next-line react-hooks/exhaustive-deps
  }, [grokCollection, patternSlotId]);

+ useEffect(() => {
+   return () => {
+     draftGrokExpression.destroy();
+   };
+ }, [draftGrokExpression]);
📝 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
const draftGrokExpression = useMemo(() => {
return new DraftGrokExpression(grokCollection, pattern);
return new DraftGrokExpression(grokCollection, pattern, { patternSlotId });
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [grokCollection]);
}, [grokCollection, patternSlotId]);
const draftGrokExpression = useMemo(() => {
return new DraftGrokExpression(grokCollection, pattern, { patternSlotId });
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [grokCollection, patternSlotId]);
useEffect(() => {
return () => {
draftGrokExpression.destroy();
};
}, [draftGrokExpression]);
🤖 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/platform/packages/shared/kbn-grok-ui/components/expression.tsx` around
lines 45 - 48, The DraftGrokExpression instance created in the useMemo
(draftGrokExpression) subscribes to customPatternsChanged$ and registers itself
as a field-usage source but is never destroyed, causing
subscription/registration leaks; update the component to ensure destroy() is
called when the component unmounts or when dependencies change (e.g., by
creating the DraftGrokExpression in the current hook and adding a useEffect that
returns a cleanup which calls draftGrokExpression.destroy()), referencing
DraftGrokExpression, draftGrokExpression, customPatternsChanged$, and destroy()
so the instance is properly unsubscribed and deregistered on teardown.


const [editorValue, setEditorValue] = useState(pattern);
const pendingLocalChangeRef = useRef(false);

// Sync pattern prop with internal DraftGrokExpression
// Sync external pattern changes without clobbering in-progress local edits.
useEffect(() => {
const currentExpression = draftGrokExpression.getExpression();
if (currentExpression !== pattern) {
if (pendingLocalChangeRef.current) {
if (pattern === draftGrokExpression.getExpression()) {
pendingLocalChangeRef.current = false;
setEditorValue(pattern);
}
return;
}
if (draftGrokExpression.getExpression() !== pattern) {
draftGrokExpression.updateExpression(pattern);
}
setEditorValue(pattern);
}, [pattern, draftGrokExpression]);

const grokEditorRef = useRef<monaco.editor.IStandaloneCodeEditor | null>(null);
const decorationsRef = useRef<monaco.editor.IEditorDecorationsCollection | null>(null);
const { containerRef, setupResizeChecker, destroyResizeChecker } = useResizeChecker();

// Monaco can't accept inline styles per-decoration; we pass class names via inlineClassName
// and inject the corresponding CSS rules onto the wrapper via Emotion so the classes resolve.
const colourPaletteStyles = useMemo(
() => grokCollection.getColourPaletteStyles(euiTheme),
[euiTheme, grokCollection]
);

const onGrokEditorMount: CodeEditorProps['editorDidMount'] = (
editor: monaco.editor.IStandaloneCodeEditor
) => {
grokEditorRef.current = editor;
decorationsRef.current = editor.createDecorationsCollection();
setupResizeChecker(editor);
updateDecorations(draftGrokExpression, grokCollection, grokEditorRef, decorationsRef);
};

const onGrokEditorWillUnmount: CodeEditorProps['editorWillUnmount'] = () => {
destroyResizeChecker();
};

const onGrokEditorChange: CodeEditorProps['onChange'] = (value) => {
pendingLocalChangeRef.current = true;
setEditorValue(value);
draftGrokExpression.updateExpression(value);
onChange?.(value);
updateDecorations(draftGrokExpression, grokCollection, grokEditorRef, decorationsRef);
};

// Re-apply decorations when the pattern prop changes externally (e.g. form state rewrites
// the value, or another consumer drives the editor).
useEffect(() => {
updateDecorations(draftGrokExpression, grokCollection, grokEditorRef, decorationsRef);
}, [pattern, draftGrokExpression, grokCollection]);

return (
<div
ref={containerRef}
css={css`
${colourPaletteStyles}
`}
style={{
width: '100%',
height,
Expand All @@ -74,7 +118,7 @@ export const Expression = ({
>
<CodeEditor
languageId="grok"
value={pattern}
value={editorValue}
height={height}
fullWidth={true}
editorDidMount={onGrokEditorMount}
Expand All @@ -86,3 +130,58 @@ export const Expression = ({
</div>
);
};

// Scans the editor's current text for `%{SYNTAX:field}` tokens and applies an inline class on
// each so the token background matches the colour assigned to that field by the resolved
// pattern (and by extension the preview-table highlight for the same field).
const updateDecorations = (
draftGrokExpression: DraftGrokExpression,
grokCollection: GrokCollection,
editorRef: React.MutableRefObject<monaco.editor.IStandaloneCodeEditor | null>,
decorationsCollectionRef: React.MutableRefObject<monaco.editor.IEditorDecorationsCollection | null>
) => {
const editor = editorRef.current;
const decorationsCollection = decorationsCollectionRef.current;
if (!editor || !decorationsCollection) return;

const model = editor.getModel();
if (!model) return;

const fields = draftGrokExpression.getFields();
// Build a field name -> colour lookup from the resolved fields. Multiple capture-group ids
// can share a field name (e.g. same field referenced from two patterns); first one wins.
const fieldColourMap = new Map<string, string>();
for (const [, fieldDef] of fields) {
if (!fieldColourMap.has(fieldDef.name)) {
fieldColourMap.set(fieldDef.name, fieldDef.colour);
}
}

const text = model.getValue();
const decorations: monaco.editor.IModelDeltaDecoration[] = [];

GROK_FIELD_PATTERN_REGEX.lastIndex = 0;
let match: RegExpExecArray | null;
while ((match = GROK_FIELD_PATTERN_REGEX.exec(text)) !== null) {
const fieldName = match[1];
const colour = fieldColourMap.get(fieldName) ?? grokCollection.lookupAssignedColour(fieldName);
if (!colour) continue;

const startPos = model.getPositionAt(match.index);
const endPos = model.getPositionAt(match.index + match[0].length);
decorations.push({
range: {
startLineNumber: startPos.lineNumber,
startColumn: startPos.column,
endLineNumber: endPos.lineNumber,
endColumn: endPos.column,
},
options: {
inlineClassName: colourToClassName(colour),
},
});
}

decorationsCollection.clear();
decorationsCollection.set(decorations);
};
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,9 @@ export const useGrokExpressions = (patterns: string[]): DraftGrokExpression[] =>

while (newExpressions.length < patterns.length) {
const idx = newExpressions.length;
newExpressions.push(new DraftGrokExpressionClass(grokCollection, patterns[idx]));
newExpressions.push(
new DraftGrokExpressionClass(grokCollection, patterns[idx], { patternSlotId: idx })
);
}

// Update expressions that have changed
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,31 +9,51 @@

import type { Subscription } from 'rxjs';
import { BehaviorSubject } from 'rxjs';
import type { GrokCollection } from './grok_collection_and_pattern';
import type { GrokCollection, GrokFieldUsageSource } from './grok_collection_and_pattern';
import { GrokPattern } from './grok_collection_and_pattern';

export class DraftGrokExpression {
export interface DraftGrokExpressionOptions {
patternSlotId?: string | number;
}

export class DraftGrokExpression implements GrokFieldUsageSource {
private expression: string = '';
private grokPattern: GrokPattern;
private expression$: BehaviorSubject<string>;
private customPatternsSubscription: Subscription;
private readonly unregisterFieldUsage: () => void;
private previousFieldNames: Set<string>;
private readonly patternSlotId: string | number | undefined;

constructor(collection: GrokCollection, initialExpression?: string) {
constructor(
collection: GrokCollection,
initialExpression?: string,
options?: DraftGrokExpressionOptions
) {
const expression = initialExpression ?? '';
this.expression = expression;
this.patternSlotId = options?.patternSlotId;
this.previousFieldNames = collection.parseFieldNames(expression);
this.unregisterFieldUsage = collection.registerFieldUsageSource(this);
this.grokPattern = new GrokPattern(expression || '', 'DRAFT_GROK_EXPRESSION', collection);
this.grokPattern.resolvePattern();
this.expression$ = new BehaviorSubject<string>(expression);
collection.flushFieldUsage();
this.customPatternsSubscription = collection.customPatternsChanged$.subscribe(() => {
this.grokPattern.resolvePattern(true);
this.expression$.next(this.expression);
});
}

public updateExpression = (expression: string) => {
const collection = this.grokPattern.getParentCollection();
const nextFieldNames = collection.parseFieldNames(expression);
collection.reconcileFieldUsage(this, this.previousFieldNames, nextFieldNames);
this.expression = expression;
this.previousFieldNames = nextFieldNames;
this.grokPattern.updatePattern(this.expression);
this.grokPattern.resolvePattern(true);
collection.flushFieldUsage();
this.expression$.next(this.expression);
};

Expand All @@ -57,11 +77,21 @@ export class DraftGrokExpression {
return this.expression;
};

public getFieldNames = (): ReadonlySet<string> => {
return this.previousFieldNames;
};

public getPatternSlotId = () => {
return this.patternSlotId;
};

public getExpression$ = () => {
return this.expression$;
};

public destroy = () => {
this.customPatternsSubscription.unsubscribe();
this.unregisterFieldUsage();
this.grokPattern.getParentCollection().flushFieldUsage();
};
}
Loading
Loading