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: Add ability to target headers by name #360

Merged
merged 4 commits into from
Nov 25, 2024

Conversation

e-fisher
Copy link
Collaborator

@e-fisher e-fisher commented Nov 22, 2024

Description

Add ability to target header by name in selectors, works both in correlation and parameterization rules.
I've added and styled ReactSelect to add searchable dropdowns, and used it to show header suggestions from recorded requests.

How to Test

  • Test header replacement using parameterization rule
  • Test header replacement using correlation rule with default replacer
  • Test header replacement using correlation rule with custom replacer

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

CleanShot 2024-11-22 at 16 55 10

Related PR(s)/Issue(s)

Resolves #307

@e-fisher e-fisher force-pushed the feat/target-headers-by-name branch from 2a7228f to 2083453 Compare November 22, 2024 14:44
Comment on lines +13 to +16
const [isSidebarExpanded, setIsSidebarExpanded] = useLocalStorage(
'isSidebarExpanded',
true
)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not related to the feature, but added this as QoL to avoid closing the sidebar every time I did cmd+r to refresh the app.

export function createCorrelationRuleInstance(
rule: CorrelationRule,
idGenerator: Generator<number>
idGenerator: IdGenerator
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Cleaning up some "unsafe any" warnings

import { ChevronDownIcon, ThickCheckIcon } from '@radix-ui/themes'
import { getStylesConfig, getThemeConfig } from './StyledReactSelect.styles'

export function StyledReactSelect<Option>(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Far from ideal integration of ReactSelect into Radix, it doesn't support all the props Radix's select does and has static size, but this iteration I've focused on blending it into Radix styles.

@e-fisher e-fisher marked this pull request as ready for review November 22, 2024 14:56
@e-fisher e-fisher requested a review from a team as a code owner November 22, 2024 14:56
@e-fisher e-fisher requested review from going-confetti and cristianoventura and removed request for a team November 22, 2024 14:56
going-confetti
going-confetti previously approved these changes Nov 22, 2024
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 🚢

export function StyledReactSelect<Option>(
props: ComponentProps<typeof Select<Option>>
) {
const styles = useMemo(() => getStylesConfig<Option>(), [])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this be moved to just before the component function? useMemo won't be needed then.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The only reason is passing that generic type, was getting TS errors without it :(

Copy link
Collaborator

@cristianoventura cristianoventura left a comment

Choose a reason for hiding this comment

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

One minor comment regarding some options in the dropdown, otherwise it looks great! 🚀

Comment on lines +16 to +18
const headers = filteredRequests.flatMap(
(request) => request?.[extractFrom]?.headers ?? []
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I noticed some headers are listed in the dropdown but not generated in the script so selecting them doesn't have any effect. We could exclude them from the dropdown as well to keep consistency.

const headersToExclude = ['Cookie', 'User-Agent', 'Host', 'Content-Length']
const headers = request.headers
.filter(([name]) => !headersToExclude.includes(name))

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good call! Updated 👌

@e-fisher e-fisher merged commit 04e4fd9 into main Nov 25, 2024
2 checks passed
@e-fisher e-fisher deleted the feat/target-headers-by-name branch November 25, 2024 14:22
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.

Allow extracting and replacing from headers key
3 participants