Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Apply rules in request list in generators #499

Merged
merged 5 commits into from
Feb 19, 2025

Conversation

e-fisher
Copy link
Collaborator

@e-fisher e-fisher commented Feb 18, 2025

Description

This is the first PR towards improved rule preview - apply rules in request list in generators. Previously, we would show unmodified requests, with exception when opening a rule editor and viewing rule preview tab, which will be removed in future PRs. Also, it wasn't possible to see request list with all rules applied, just the one you have selected.

Per suggestion during our latest sync I've also added ability to switch between original request list and the one with rules applied. It affects request list, details inspector, script, and validator. This may come handy when debugging rules and user won't have to disable rules one-by-one.

How to Test

Verify modifications added by rules are reflected in request list, inspector, script, and validator. Confirm with both "apply rules" toggle enabled and disabled.

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):

screencapture.2025-02-18.at.14.34.44.mp4

Related PR(s)/Issue(s)

@e-fisher e-fisher marked this pull request as ready for review February 18, 2025 12:51
@e-fisher e-fisher requested a review from a team as a code owner February 18, 2025 12:51
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it expected to still have the Rule preview tab? Personally, I find it more confusing to have preview in two places and to be able to turn off rule application while being in the rule editor

}

return (
<Flex
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't prevent the user from using keyboard to edit the rules, but I'm not convinced we should disable/hide the UI if the switch is off - maybe keeping the rule list/rule editor accessible, but showing a very noticeable Callout component at the top of that pane will work better? Here's what it could look like (the copy is a quick draft)

image image

Might be worth discussing this with @majavojnoska

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agree, this looks better 🙌 Replaced overlay with callout:

screencapture 2025-02-19 at 14 46 38

@@ -4,6 +4,7 @@ import { ImmerStateCreator } from '@/utils/typescript'
interface State {
rules: TestRule[]
selectedRuleId: string | null
rulesEnabled: boolean
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it should have a different name, because we already have a concept of disabled rules and it means something else.

checked={rulesEnabled}
onCheckedChange={setEnableRules}
/>
Apply rules
Copy link
Collaborator

Choose a reason for hiding this comment

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

To me, this part of the new UI is rather confusing - why would I not want to apply rules to my recording?
I also think that when it comes to script generation rules should be applied regardless of this setting.

I don't have a good copy suggestion, but maybe something focused on the recording rather than rules? The reason I think it might be better is because the goal of having this switch (as far as I understand) is for the user to be able to check the original recording without any test rules applied.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've used "View original requests" and will revisit after we collect feedback

@e-fisher
Copy link
Collaborator Author

Is it expected to still have the Rule preview tab? Personally, I find it more confusing to have preview in two places and to be able to turn off rule application while being in the rule editor

Keeping the preview tab in this iteration to preserve the ability to see which requests are affected by the selected rule. Will be removed in the next PR.

Other comments have been addressed, and it's ready for another review 🙌

Copy link
Collaborator

@going-confetti going-confetti left a comment

Choose a reason for hiding this comment

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

LGTM!

@e-fisher e-fisher merged commit d14fe68 into main Feb 19, 2025
3 checks passed
@e-fisher e-fisher deleted the feat/apply-rules-in-request-list branch February 19, 2025 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants