[Streams] Match grok pattern color with preview highlighting#269680
[Streams] Match grok pattern color with preview highlighting#269680mohamedhamed-ahmed wants to merge 11 commits into
Conversation
mohamedhamed-ahmed
added
the
Feature:Streams
|
Pinging @elastic/obs-onboarding-team (Team:obs-onboarding) |
mohamedhamed-ahmed
added
release_note:skip
There was a problem hiding this comment.
This works well, one thing I'm not sure about is that we don't reuse color codes, which causes this (color cycles each key press):
Besides the slightly weird visual, the effect of that is that the color assignments won't end up evenly distributed in the end, at least it's not guaranteed:
Should we make it slightly smarter and kick out and reuse colors for group names that disappear? I think it would be cool to make the first group color x, the second group color y and so on in a consistent manner, if that makes sense
| @@ -184,6 +195,7 @@ export class GrokCollection { | |||
|
|
|||
| public resetColourIndex = () => { | |||
There was a problem hiding this comment.
It seems like we don't need resetColourIndex anymore?
|
@mohamedhamed-ahmed I'm not sure how much time we should spend on it, it's really a balancing act - if we use the index of the named group for the color assignment, then they can change quite a bit if you add something in between, which is also not great. If @3kt is happy I'm happy too. |
|
The goal is to get beyond "just feature parity" for such a widely used processor: I want users to get a compelling reason to use Streams UI rather than anything else (plus it demos quite well). Ideally let's solve the color cycling then we're good to go. |
@3kt you mean the typing issue |
|
@mohamedhamed-ahmed yes - IIRC my attempt suffered from the same issue. |
Yes tested it there as well, I already have a fix will test it now and push the changes |
|
@flash1293 this is how the behavior is now. Screen.Recording.2026-05-21.at.13.18.21.mov |
|
Darn this looks great @mohamedhamed-ahmed <3 |
|
Colors are still behaving a bit weirdly: 
But not a blocker I'd say. In general it feels like the UI is a bit laggy when using grok highlighting in general, @Kerry350 do you have any thoughts about this? Is this something that has room for optimization? Maybe more aggressive debounce? I don't mind if it takes a second to propagate a new pattern, but it's really annoying when typing and the characters don't appear. Also an issue on main though |
Off the top of my head there's probably room to debounce better when doing the actual parsing, application of highlights in Monaco etc. We can probably separate that process from the process of "just" updating the actual input value. It might be harder than it sounds as it will cause character drift on the highlights already applied (if that makes sense), unless we're okay with something like clearing the highlights until the user is done typing. When I did the refactor to providers for easier consumer use the performance was okay, so maybe something changed along the way. Worth an issue to investigate at least. |
📝 WalkthroughWalkthroughThis PR implements stable field-name-based color assignment for grok pattern editing. 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a 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: 2
🤖 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/platform/packages/shared/kbn-grok-ui/components/expression.tsx`:
- Around line 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.
In
`@x-pack/platform/plugins/shared/streams_app/public/components/stream_management/data_management/stream_detail_enrichment/steps/blocks/action/grok/grok_patterns_editor.tsx`:
- Around line 195-212: The file uses the EuiToolTip component but it isn't
imported, causing a runtime error; update the import statement that destructures
components from '`@elastic/eui`' to include EuiToolTip (alongside existing imports
like EuiButtonIcon) in grok_patterns_editor.tsx so the EuiToolTip symbol is
available where the tooltip wrapping the remove button (onRemove handler /
EuiButtonIcon) is rendered.
🪄 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 5ba4b7dd-ac53-417b-ab44-106525d09ba2
📒 Files selected for processing (6)
src/platform/packages/shared/kbn-grok-ui/components/expression.tsxsrc/platform/packages/shared/kbn-grok-ui/contexts/grok_expressions_context.tsxsrc/platform/packages/shared/kbn-grok-ui/models/draft_grok_expression.tssrc/platform/packages/shared/kbn-grok-ui/models/grok_collection_and_pattern.tssrc/platform/packages/shared/kbn-grok-ui/models/grok_models.test.tsx-pack/platform/plugins/shared/streams_app/public/components/stream_management/data_management/stream_detail_enrichment/steps/blocks/action/grok/grok_patterns_editor.tsx
| 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]); |
There was a problem hiding this comment.
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.
| 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.
| <EuiToolTip | ||
| content={i18n.translate( | ||
| 'xpack.streams.streamDetailView.managementTab.enrichment.processor.grokEditor.removePattern', | ||
| { defaultMessage: 'Remove grok pattern' } | ||
| )} | ||
| /> | ||
| disableScreenReaderOutput | ||
| > | ||
| <EuiButtonIcon | ||
| data-test-subj="streamsAppDraggablePatternInputButton" | ||
| iconType="minusCircle" | ||
| color="danger" | ||
| onClick={onRemove} | ||
| aria-label={i18n.translate( | ||
| 'xpack.streams.streamDetailView.managementTab.enrichment.processor.grokEditor.removePattern', | ||
| { defaultMessage: 'Remove grok pattern' } | ||
| )} | ||
| /> | ||
| </EuiToolTip> |
There was a problem hiding this comment.
EuiToolTip is used but not imported – runtime error.
Add EuiToolTip to the destructured imports from @elastic/eui on line 11.
Proposed fix
import {
EuiFormRow,
EuiPanel,
EuiButtonEmpty,
EuiDraggable,
EuiFlexGroup,
EuiIcon,
EuiButtonIcon,
EuiFlexItem,
useEuiTheme,
+ EuiToolTip,
} from '`@elastic/eui`';📝 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.
| <EuiToolTip | |
| content={i18n.translate( | |
| 'xpack.streams.streamDetailView.managementTab.enrichment.processor.grokEditor.removePattern', | |
| { defaultMessage: 'Remove grok pattern' } | |
| )} | |
| /> | |
| disableScreenReaderOutput | |
| > | |
| <EuiButtonIcon | |
| data-test-subj="streamsAppDraggablePatternInputButton" | |
| iconType="minusCircle" | |
| color="danger" | |
| onClick={onRemove} | |
| aria-label={i18n.translate( | |
| 'xpack.streams.streamDetailView.managementTab.enrichment.processor.grokEditor.removePattern', | |
| { defaultMessage: 'Remove grok pattern' } | |
| )} | |
| /> | |
| </EuiToolTip> | |
| import { | |
| EuiFormRow, | |
| EuiPanel, | |
| EuiButtonEmpty, | |
| EuiDraggable, | |
| EuiFlexGroup, | |
| EuiIcon, | |
| EuiButtonIcon, | |
| EuiFlexItem, | |
| useEuiTheme, | |
| EuiToolTip, | |
| } from '`@elastic/eui`'; |
🤖 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
`@x-pack/platform/plugins/shared/streams_app/public/components/stream_management/data_management/stream_detail_enrichment/steps/blocks/action/grok/grok_patterns_editor.tsx`
around lines 195 - 212, The file uses the EuiToolTip component but it isn't
imported, causing a runtime error; update the import statement that destructures
components from '`@elastic/eui`' to include EuiToolTip (alongside existing imports
like EuiButtonIcon) in grok_patterns_editor.tsx so the EuiToolTip symbol is
available where the tooltip wrapping the remove button (onRemove handler /
EuiButtonIcon) is rendered.
|
@flash1293 @Kerry350 Do you think we can merge it with the current behavior and open a separats ticket for the debounce enhancement? |
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Async chunks
History
|
5 participants
closes https://github.com/elastic/streams-program/issues/1197
Summary
This PR adds similar color matching for the grok pattern and the preview highlighting