feat: Display rule summary preview while editing a rule #607

fsd-niraj · 2025-03-27T00:30:23Z

Added TestRule to RuleEditor to display rule summary while editing rule.
Changed minor CSS to the affected components.

Description

How to Test

Checklist

I have performed a self-review of my code.
I have added tests for my changes.
I have run linter locally (npm run lint) and all checks pass.
I have run tests locally (npm test) and all tests pass.
I have commented on my code, particularly in hard-to-understand areas.

Screenshots (if appropriate):

Related PR(s)/Issue(s)

Resolves #606

…nd used it wherever necessary

CLAassistant · 2025-03-27T00:30:30Z

All committers have signed the CLA.

going-confetti

Hi @fsd-niraj

Thank you for this contribution!

I believe a few things are missing in terms of functionality:

Support for all types of test rules (not just Parameterization)
Rule filter isn't shown

I left a couple of comments that should help you address these. Please let me know if you have any questions.

Additionally, we haven't added proper contribution guidelines yet, but we follow the Conventional Commits spec, so could you please rename you PR accordingly (I suggest feat: Display rule summary preview while editing a rule)?

going-confetti · 2025-03-27T15:47:09Z

src/views/Generator/RuleEditor/ParameterizationEditor/ParameterizationEditor.tsx

  return (
    <>
      <Text size="2" as="p" mb="2" color="gray">
-        Replace request data with variables or custom values.
+        <ParameterizationSelectorContent rule={rule} />


We would like to have a preview for all existing types of test rules. Instead of updating each type of the rule rule editor, could you please move it to StickyPanelHeader in the RuleEditor component?

I'm not too sure about the alignment, but here's an example with the preview being on the right:

fsd-niraj · 2025-03-27T19:51:44Z

Thanks for the review and feedback @going-confetti .

I have moved the component as desired. It is all coming from RuleEditor.
I have also added small UI fixes on the same screen, let me know if those are accepted in the following PR.
I'm pushing it on the same branch.

…#606

going-confetti

Thank you for the changes. This placement is correct.

I don't necessarily agree with the proposed UI fixes, as the positions of the New rule button and the empty message are intentional and changing them is out of scope of this PR.

I added a comment to your changes in RuleEditor.tsx - addressing it should make the PR smaller

going-confetti · 2025-03-31T09:51:49Z

src/views/Generator/TestRuleContainer/TestRuleContainer.tsx

-        px="6"
+        px="3"


Not sure if this needs to be changed in this PR, as it is out of scope of the original issue.
The reason it's 6 is to align it with Rule type labels - we may reconsider this in future, but I don't think it's a bug.

going-confetti · 2025-03-31T09:55:14Z

src/views/Generator/TestRuleContainer/TestRuleContainer.tsx

+          <Flex justify="center" align="center" m="auto">
+            <EmptyMessage
+              message="Configure your test logic by adding a new rule"
+              pb="2"
+              action={<NewRuleMenu variant="solid" size="2" color="orange" />}
+            />
+          </Flex>


Displaying the EmptyMessage component at the top of the parent is intentional, so this change isn't needed.

src/views/Generator/TestRuleContainer/TestRule/TestRuleSelector.tsx

going-confetti · 2025-03-31T09:58:41Z

src/views/Generator/TestRuleContainer/TestRule/VerificationContent.tsx

See my comment in RuleEditor.tsx

going-confetti · 2025-03-31T09:58:54Z

src/views/Generator/TestRuleContainer/TestRule/TestRuleInlineContent.tsx

See my comment in RuleEditor.tsx

going-confetti · 2025-03-31T10:07:22Z

src/views/Generator/RuleEditor/RuleEditor.tsx

+          <Flex align="center" justify="between">
+            <Flex align="center" gap="3" width="100%">
+              <Heading size="2" weight="medium">
+                {capitalize(startCase(rule.type))}
+              </Heading>
+              <Button
+                onClick={handleClose}
+                variant="ghost"
+                size="1"
+                css={{ gap: 0 }}
+              >
+                <ChevronLeftIcon />
+                Back
+              </Button>
+            </Flex>
+            <TestRuleInlineContent rule={rule} />


Instead of adding this extra Flex wrapper here, may I ask you to wrap only TestRuleInlineContent with a Flex? Aligning it to the right can be achieved by adding ml="auto".
This should remove the need for changes in TestRuleInlineContent.tsx and VerificationContent.tsx that may have side-effects in other places.

I get what you mean, I'll fix that in next PR.

src/views/Generator/TestRuleContainer/StickyPanelHeader.tsx

going-confetti · 2025-03-31T15:52:26Z

src/views/Generator/TestRuleContainer/TestRule/TestRuleInlineContent.tsx

@@ -39,43 +39,49 @@ export function TestRuleInlineContent({ rule }: TestRuleInlineContentProps) {
 function CorrelationContent({ rule }: { rule: CorrelationRule }) {
  return (
    <>
-      <TestRuleFilter filter={rule.extractor.filter} />
-      <TestRuleSelector rule={rule} />
+      <Flex gap="2" align="center">


These extra Flex wrappers are not needed here and could potentially have unwanted side-effects in TestRule.tsx where TestRuleInlineContent is also used.

You can revert changes to both this file and VerificationContent.tsx as long as TestRuleInlineContent has a proper wrapper in RuleEditor.tsx

If Flex is removed, the Correlation section is going to get affected like this.

My suggestion is to basically have that Flex in RuleEditor. It will likely need flexGrow to be set to 1 to make sure it can take up the full available space if needed

going-confetti · 2025-03-31T15:59:48Z

src/views/Generator/RuleEditor/RuleEditor.tsx

@@ -100,7 +101,7 @@ export function RuleEditor({ rule }: RuleEditorProps) {
    <ScrollArea scrollbars="vertical">
      <FormProvider {...formMethods}>
        <StickyPanelHeader>
-          <Flex align="center" gap="3">
+          <Flex align="center" gap="3" width="100%" ml="auto">


The position itself was correct in the previous commit, these properties mean there's no place for TestRuleInlineContent on this "line", which results in a somewhat broken layout

This is what it should look like:

Check how TestRuleInlineContent is used in TestRule.tsx - if you wrap it with a Flex and put it inside theFlex on line 104, you should be able to achieve this (it will need some extra props to make sure it's aligned to the right)

I think this was correct in my previous PR. Nevermind, reverted back.

cristianoventura

Thanks for your contribution @fsd-niraj!

Functionality-wise, it looks good! Just a couple of UI fixes as already requested. Also, we need to ensure that the UI of the original list of rules is not affected, as I mentioned in my comment.

cristianoventura · 2025-04-01T22:19:35Z

src/views/Generator/TestRuleContainer/TestRule/VerificationContent.tsx

-        Verify <Strong>{rule.target}</Strong>{' '}
-        <OperatorLabel operator={rule.operator} /> <ValueLabel rule={rule} />
-      </Badge>
+      <Flex gap="2" align="center">


I noticed that the addition of this Flex wrapper made the original rules list truncate

I can continue working on it

exported ParameterizationSelectorContent fu, pulled rules via props a…

968ebe0

…nd used it wherever necessary

fsd-niraj requested a review from a team as a code owner March 27, 2025 00:30

fsd-niraj requested review from going-confetti and cristianoventura March 27, 2025 00:30

fsd-niraj changed the title ~~exported ParameterizationSelectorContent fu, pulled rules via props a…~~ exported ParameterizationSelectorContent fn, pulled rules via props a… Mar 27, 2025

going-confetti requested changes Mar 27, 2025

View reviewed changes

fsd-niraj changed the title ~~exported ParameterizationSelectorContent fn, pulled rules via props a…~~ feat: Display rule summary preview while editing a rule Mar 27, 2025

feat: feat: Display rule summary preview while editing a rule grafana…

311d956

…#606

fsd-niraj requested a review from going-confetti March 28, 2025 12:49

going-confetti requested changes Mar 31, 2025

View reviewed changes

removed unnecessary exports, flex and padding.

fd416a9

fsd-niraj requested a review from going-confetti March 31, 2025 15:22

going-confetti requested changes Mar 31, 2025

View reviewed changes

cristianoventura reviewed Apr 1, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Display rule summary preview while editing a rule #607

feat: Display rule summary preview while editing a rule #607

fsd-niraj commented Mar 27, 2025 •

edited

Loading

CLAassistant commented Mar 27, 2025 •

edited

Loading

going-confetti left a comment

going-confetti Mar 27, 2025

fsd-niraj commented Mar 27, 2025 •

edited

Loading

going-confetti left a comment

going-confetti Mar 31, 2025

going-confetti Mar 31, 2025

going-confetti Mar 31, 2025

going-confetti Mar 31, 2025

going-confetti Mar 31, 2025

fsd-niraj Mar 31, 2025

going-confetti Mar 31, 2025

fsd-niraj Mar 31, 2025

going-confetti Apr 1, 2025

going-confetti Mar 31, 2025

fsd-niraj Mar 31, 2025

cristianoventura left a comment

cristianoventura Apr 1, 2025 •

edited

Loading

fsd-niraj Apr 17, 2025

feat: Display rule summary preview while editing a rule #607

Are you sure you want to change the base?

feat: Display rule summary preview while editing a rule #607

Conversation

fsd-niraj commented Mar 27, 2025 • edited Loading

Description

How to Test

Checklist

Screenshots (if appropriate):

Related PR(s)/Issue(s)

CLAassistant commented Mar 27, 2025 • edited Loading

going-confetti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fsd-niraj commented Mar 27, 2025 • edited Loading

going-confetti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cristianoventura left a comment

Choose a reason for hiding this comment

cristianoventura Apr 1, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fsd-niraj commented Mar 27, 2025 •

edited

Loading

CLAassistant commented Mar 27, 2025 •

edited

Loading

fsd-niraj commented Mar 27, 2025 •

edited

Loading

cristianoventura Apr 1, 2025 •

edited

Loading