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: Update WebLogView to handle common mime types #41

Merged
merged 9 commits into from
Jul 19, 2024

Conversation

2Steaks
Copy link
Collaborator

@2Steaks 2Steaks commented Jul 17, 2024

Closes https://github.com/grafana/k6-cloud/issues/2389

This PR updates the WebLogView

  • Handle various common mime types
  • Add split pane for request/response
  • Add rendering/raw content views
  • Add cookies view
Screen.Recording.2024-07-17.at.14.25.16.mov

Questions

  • Do we want to start using emotion for all styling?
  • Should we hide tabs that don't have content?

- Update Remove padding from Drawer component
- Update WebLogView/Row/SideBar layout
- Add RequestDetails components
- Add ResponseDetails components
- Add a Tabs component to fill the height of the Allotment component
@2Steaks 2Steaks self-assigned this Jul 17, 2024
@2Steaks 2Steaks marked this pull request as ready for review July 17, 2024 18:56
@2Steaks 2Steaks requested review from going-confetti and e-fisher and removed request for going-confetti July 17, 2024 18:56
Copy link
Collaborator

@e-fisher e-fisher left a comment

Choose a reason for hiding this comment

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

This looks so much better 💯

There's some duplication in the code where I left comments, but overall looks good!

Few things regarding UI/UX:

  1. There's no longer a close button in the drawer - no way to hide it
  2. I think one important mime type missed is form data, that must be quite common. We have an example in test.k6.io:

CleanShot 2024-07-18 at 10 36 06@2x

  1. Can we add another label to the switch to make it more clear what it switches between?

CleanShot 2024-07-18 at 10 30 38@2x

Comment on lines 5 to 35
export function Cookies({ data }: { data: ProxyData }) {
const cookies = data.request.cookies || []

if (!cookies.length) {
return (
<Flex height="200px" justify="center" align="center">
Cookies not available
</Flex>
)
}

return (
<Table.Root size="1" variant="surface">
<Table.Header>
<Table.Row>
<Table.ColumnHeaderCell>Name</Table.ColumnHeaderCell>
<Table.ColumnHeaderCell>Value</Table.ColumnHeaderCell>
</Table.Row>
</Table.Header>

<Table.Body>
{cookies.map(([name, value], index) => (
<Table.Row key={index}>
<Table.Cell>{name}</Table.Cell>
<Table.Cell>{value}</Table.Cell>
</Table.Row>
))}
</Table.Body>
</Table.Root>
)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Request and response Cookies components are almost the same, instead of passing ProxyData you could pass just the cookies (either request's or response's) and reuse the same component

Comment on lines 9 to 31
const options: editor.IStandaloneEditorConstructionOptions = {
automaticLayout: true,
codeLens: false,
contextmenu: false,
domReadOnly: true,
fixedOverflowWidgets: true,
foldingMaximumRegions: 5,
lineNumbers: 'off',
minimap: {
enabled: false,
renderCharacters: false,
},
overviewRulerBorder: false,
readOnly: true,
scrollbar: {
alwaysConsumeMouseWheel: true,
horizontalScrollbarSize: 3,
verticalScrollbarSize: 3,
},
scrollBeyondLastLine: false,
tabSize: 1,
wordWrap: 'off',
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Duplicate options and theme switching logic between Payload and Preview, that's a good candidate for a shared component

import { Tabs as RadixTabs } from '@radix-ui/themes'
import { ComponentProps } from 'react'

function Root({ children, ...props }: ComponentProps<typeof RadixTabs.Root>) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wasn't sure why we need tabs wrapper at a glance, maybe it would be useful to have a comment here?

@2Steaks
Copy link
Collaborator Author

2Steaks commented Jul 18, 2024

Can we add another label to the switch to make it more clear what it switches between?

I've opted for the segmented control component instead.

Screenshot 2024-07-18 at 11 17 32

- Consolidate Cookie component
- Consolidate Read-only editor component
- Update raw/preview switch to segmented controls
@2Steaks
Copy link
Collaborator Author

2Steaks commented Jul 18, 2024

I think one important mime type missed is form data, that must be quite common

Thank you for finding the example request!

Screenshot 2024-07-18 at 11 51 20

@2Steaks 2Steaks requested a review from e-fisher July 18, 2024 11:10
Copy link
Collaborator

@e-fisher e-fisher left a comment

Choose a reason for hiding this comment

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

Still says "payload not available" for some reason 🤔 did you push all changes?

CleanShot 2024-07-18 at 14 58 22@2x

export function Preview({ content, contentType, format }: PreviewProps) {
if (format === 'image') {
return (
<img
Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs a fallback:
image
Depending on how easy it is to implement it, we may want to create a separate issue for that work

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like it's broken for PNGs and SVGs - I can share the HAR I used if you want

case 'plain':
return safeAtob(content)
case 'audio':
case 'font':
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe this should also be safeAtob(content)?
image
image

variant="classic"
onValueChange={(value) => setIsPreview(value === 'preview')}
>
<SegmentedControl.Item value="raw">Raw</SegmentedControl.Item>
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 for some MIME types (like images), we shouldn't let the user see the raw response

@2Steaks 2Steaks requested a review from e-fisher July 19, 2024 09:15
- Disable raw content for media files
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!

@2Steaks 2Steaks merged commit 6326a18 into main Jul 19, 2024
1 check passed
@2Steaks 2Steaks deleted the feat/weblogview-mime-types branch July 19, 2024 12: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.

3 participants