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.

Sign up for GitHub

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: Recording selector draft #36

Merged
merged 6 commits into from
Jul 17, 2024
Merged

feat: Recording selector draft #36

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6f794bd
feat: Recording selector draft
going-confetti Jul 16, 2024
33d22fe
Make generator name more unique
going-confetti Jul 16, 2024
d346e06
cr
going-confetti Jul 17, 2024
9f102f0
eslint fixes
going-confetti Jul 17, 2024
56cdc3a
Merge remote-tracking branch 'origin/main' into feat/recording-selector
going-confetti Jul 17, 2024
7304ae4
mor eeslint fixes
going-confetti Jul 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 12 additions & 6 deletions src/components/Layout/PageHeading.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { HomeIcon } from '@radix-ui/react-icons'
import { Flex, Heading, IconButton, Separator } from '@radix-ui/themes'
import { Flex, Heading, IconButton } from '@radix-ui/themes'
import { useNavigate } from 'react-router-dom'
import { ThemeSwitcher } from '../ThemeSwitcher'

Expand All @@ -14,7 +14,15 @@ export function PageHeading({

return (
<>
<Flex gap="2" align="center" p="2">
<Flex
gap="2"
align="center"
p="2"
style={{
borderBottom: '1px solid var(--gray-4)',
backgroundColor: 'var(--gray-2)',
}}
>
<IconButton
onClick={() => {
navigate('/')
Expand All @@ -24,15 +32,13 @@ export function PageHeading({
<HomeIcon width="18" height="18" />
</IconButton>
<Flex maxWidth="50%" flexGrow="1">
<Heading>{text}</Heading>
<Heading size="3">{text}</Heading>
</Flex>
<Flex maxWidth="50%" flexGrow="1" justify="end" align="center" gap="2">
<Flex flexGrow="1" justify="end" align="center" gap="2">
<ThemeSwitcher />
{children}
</Flex>
</Flex>

<Separator size="4" style={{ backgroundColor: 'var(--gray-4)' }} />
</>
)
}
4 changes: 4 additions & 0 deletions src/hooks/useGeneratorStore/selectors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,7 @@ export function useSelectedRule() {
return rule
})
}

export function useHasRecording() {
return useGeneratorStore((state) => state.requests.length > 0)
}
Comment on lines +20 to +22
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@e-fisher what's your favorite way of defining reusable selectors? Maybe the should be callbacks that we then import and pass to useGeneratorStore?

Copy link
Member

Choose a reason for hiding this comment

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

pmndrs/zustand#387

would this approach work well ? Seems like a clean way to have a file defining selectors, importing them and using with the useGeneratorStore
image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes! That's what I meant by callbacks that we then import and pass to useGeneratorStore, I should've been more specific

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd probably vote for the callback approach just because it's a bit more explicit -
useHasRecording - hook named this way could be doing anything
useGeneratorStore(selectHasRecording) - indicates that value is read from store

8 changes: 7 additions & 1 deletion src/hooks/useGeneratorStore/useGeneratorStore.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ export const useGeneratorStore = create<GeneratorState>()(
...createRulesSlice(set, get, store),
...createVariablesSlice(set, get, store),
...createThinkTimeSlice(set, get, store),
name: `generator_${Date()}`,
name: generateNewName(),
setName: (name: string) =>
set((state) => {
state.name = name
Expand All @@ -30,6 +30,7 @@ export const useGeneratorStore = create<GeneratorState>()(
resetRecording: () =>
set((state) => {
state.requests = []
state.recordingPath = ''
state.filteredRequests = []
state.allowList = []
}),
Expand Down Expand Up @@ -58,3 +59,8 @@ export const useGeneratorStore = create<GeneratorState>()(
}),
}))
)

function generateNewName() {
const date = new Date().toISOString().split('T')[0]
Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't split it, so that if you are working on multiple ones you can save them with a different name on the same day 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops, you're right! A fix is incoming in a bit 👀

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed it, but used the US format 🤔. We should probably use the same format we use in GCk6 for test names

Copy link
Member

Choose a reason for hiding this comment

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

I think format is a 🔥 topic, using US format would add unneeded confusion 🤔

Maybe the ideal is to use the common syntax of YYYY-MM-DD ? What do you think ?

You could argue that the format you are using is more readable, so I'm not completely sure. I would still slightly prefer the format that is readable out of the box for both US & Europe, but maybe we can gather more feedback.

Remembering that this is just the generator file name, I don't think we have a strong need for keeping the format used in test names 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

using US format would add unneeded confusion 🤔

That's fair

Maybe the ideal is to use the common syntax of YYYY-MM-DD ? What do you think ?

I'd rather use a format we use in other parts of k6 offerings, for consistency reasons

Remembering that this is just the generator file name, I don't think we have a strong need for keeping the format used in test names 🤔

As of now, it's not actually a part of the filename, but I did think about adding it to the filename

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Llandy3d I don't have a strong opinion actually, let's discuss it some time next week and pick one format 👍

return `Generator ${date}`
}
13 changes: 13 additions & 0 deletions src/hooks/useWindowTitle.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import { useEffect } from 'react'

const defaultTitle = 'k6 studio'

export function useWindowTitle(title: string) {
going-confetti marked this conversation as resolved.
Show resolved Hide resolved
useEffect(() => {
document.title = `${defaultTitle} - ${title}`

return () => {
document.title = defaultTitle
}
}, [title])
}
11 changes: 5 additions & 6 deletions src/views/Generator/AllowList/AllowList.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,11 @@ export function AllowList() {

return (
<>
<Button
onClick={() => setShowAllowListDialog(true)}
disabled={requests.length === 0}
>
Allowed hosts [{allowList.length}/{hosts.length}]
</Button>
{requests.length > 0 && (
<Button onClick={() => setShowAllowListDialog(true)}>
Allowed hosts [{allowList.length}/{hosts.length}]
</Button>
)}
Comment on lines +32 to +36
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 needed until we have the recording

{/* Radix does not unmount dialog on close, this is need to clear local state */}
{showAllowListDialog && (
<AllowListDialog
Expand Down
39 changes: 13 additions & 26 deletions src/views/Generator/Generator.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,39 +4,28 @@ import { useEffect } from 'react'

import { exportScript, saveScript, saveGenerator } from './Generator.utils'
import { PageHeading } from '@/components/Layout/PageHeading'
import { harToProxyData } from '@/utils/harToProxyData'
import { GeneratorDrawer } from './GeneratorDrawer'
import { GeneratorSidebar } from './GeneratorSidebar'
import { useGeneratorStore } from '@/hooks/useGeneratorStore'
import { useGeneratorStore, useHasRecording } from '@/hooks/useGeneratorStore'
import { TestRuleContainer } from './TestRuleContainer'
import { AllowList } from './AllowList/AllowList'
import { RecordingSelector } from './RecordingSelector'
import { useWindowTitle } from '@/hooks/useWindowTitle'

export function Generator() {
const {
rules,
setRecording,
resetRecording,
filteredRequests,
setRecordingPath,
} = useGeneratorStore()

const hasRecording = filteredRequests.length > 0
const rules = useGeneratorStore((store) => store.rules)
const name = useGeneratorStore((store) => store.name)
const resetRecording = useGeneratorStore((store) => store.resetRecording)
const filteredRequests = useGeneratorStore((store) => store.filteredRequests)
const hasRecording = useHasRecording()
useWindowTitle(name)

useEffect(() => {
return () => {
resetRecording()
}
}, [resetRecording])

const handleImport = async () => {
const harFile = await window.studio.har.openFile()
if (!harFile) return

const proxyData = harToProxyData(harFile.content)
setRecording(proxyData)
setRecordingPath(harFile.path)
}

const handleExport = async () => {
const script = await exportScript(filteredRequests, rules)

Expand All @@ -46,12 +35,10 @@ export function Generator() {
return (
<>
<PageHeading text="Generator">
<Button onClick={saveGenerator}>Save</Button>
<Button onClick={handleImport}>Import HAR</Button>
<RecordingSelector />
<AllowList />
<Button onClick={handleExport} disabled={!hasRecording}>
Export script
</Button>
{hasRecording && <Button onClick={saveGenerator}>Save</Button>}
{hasRecording && <Button onClick={handleExport}>Export script</Button>}
Comment on lines -49 to +41
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Took the liberty of rearranging the buttons to have the most important ones on the right + hiding save, export, and allow list until there's an actual recording.
It still looks a bit crowded, but probably the biggest UX issue is that primary buttons are overused in the header. We'll probably tackle this once the design is ready
image

</PageHeading>
<Allotment defaultSizes={[3, 1]}>
<Allotment.Pane minSize={400}>
Expand All @@ -64,7 +51,7 @@ export function Generator() {
</Allotment.Pane>
</Allotment>
</Allotment.Pane>
<Allotment.Pane minSize={300}>
<Allotment.Pane minSize={300} visible={hasRecording}>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Similar reasoning as with the header buttons: no need to show the sidebar if it's empty

<GeneratorSidebar requests={filteredRequests} />
</Allotment.Pane>
</Allotment>
Expand Down
9 changes: 6 additions & 3 deletions src/views/Generator/GeneratorSidebar/GeneratorSidebar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,14 @@ import { ProxyData } from '@/types'
import { Box, Flex, ScrollArea, Tabs } from '@radix-ui/themes'
import { ScriptPreview } from './ScriptPreview'
import { groupProxyData } from '@/utils/groups'
import { useHasRecording } from '@/hooks/useGeneratorStore'

interface GeneratorSidebarProps {
requests: ProxyData[]
}

export function GeneratorSidebar({ requests }: GeneratorSidebarProps) {
const hasRecording = requests.length > 0
const hasRecording = useHasRecording()
const groupedProxyData = groupProxyData(requests)

return (
Expand All @@ -21,9 +22,11 @@ export function GeneratorSidebar({ requests }: GeneratorSidebarProps) {
}}
>
<Tabs.List>
<Tabs.Trigger value="requests">Requests</Tabs.Trigger>
<Tabs.Trigger value="requests">
Requests ({requests.length})
</Tabs.Trigger>
<Tabs.Trigger value="script" disabled={!hasRecording}>
Script
Script preview
</Tabs.Trigger>
</Tabs.List>
<Tabs.Content
Expand Down
53 changes: 53 additions & 0 deletions src/views/Generator/RecordingSelector.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
import { useGeneratorStore, useHasRecording } from '@/hooks/useGeneratorStore'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Some unused imports in this file, maybe we could add this plugin to clear imports automatically (we already run eslint --fix on commit)
https://www.npmjs.com/package/eslint-plugin-unused-imports

import { harToProxyData } from '@/utils/harToProxyData'
import { css } from '@emotion/react'

Check warning on line 3 in src/views/Generator/RecordingSelector.tsx

View workflow job for this annotation

GitHub Actions / build 

'css' is defined but never used
import { ArrowDownIcon, CaretDownIcon } from '@radix-ui/react-icons'

Check warning on line 4 in src/views/Generator/RecordingSelector.tsx

View workflow job for this annotation

GitHub Actions / build 

'ArrowDownIcon' is defined but never used
import { Button, Flex, IconButton, Popover, Text } from '@radix-ui/themes'
import { startRecording } from '../Recorder/Recorder.utils'

Check warning on line 6 in src/views/Generator/RecordingSelector.tsx

View workflow job for this annotation

GitHub Actions / build 

'startRecording' is defined but never used
import { Link } from 'react-router-dom'

export function RecordingSelector() {
const recordingPath = useGeneratorStore((store) => store.recordingPath)
const setRecording = useGeneratorStore((store) => store.setRecording)
const setRecordingPath = useGeneratorStore((store) => store.setRecordingPath)
const hasRecording = useHasRecording()

Check warning on line 13 in src/views/Generator/RecordingSelector.tsx

View workflow job for this annotation

GitHub Actions / build 

'hasRecording' is assigned a value but never used

const fileName = recordingPath?.split('/').pop()

const handleImport = async () => {
const harFile = await window.studio.har.openFile()
if (!harFile) return

const proxyData = harToProxyData(harFile.content)
setRecording(proxyData)
setRecordingPath(harFile.path)
}

return (
<Flex align="center" gap="2">
<Text>{fileName || 'Select recording'}</Text>
<Popover.Root>
<Popover.Trigger>
<IconButton variant="outline">
<CaretDownIcon />
</IconButton>
</Popover.Trigger>
<Popover.Content size="1" align="end">
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This will have a scrollable/searchable list of saved recordings once we implement filesystem integration and all that

<Flex direction="column" gap="2">
<Text>You don{"'"}t have saved recordings</Text>
<Flex gap="2">
<Popover.Close>
<Button variant="outline" asChild>
<Link to="/recorder">Start recording</Link>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Currently, just redirects to the recorder page, but should also start a new recording in future

</Button>
</Popover.Close>
<Popover.Close>
<Button onClick={handleImport}>Import HAR</Button>
</Popover.Close>
</Flex>
</Flex>
</Popover.Content>
</Popover.Root>
</Flex>
)
}
2 changes: 1 addition & 1 deletion src/views/Generator/TestRuleContainer/TestRule/TestRuleActions.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ export function TestRuleActions({ ruleId }: TestRuleActionsProps) {
</IconButton>
</DropdownMenu.Trigger>
<DropdownMenu.Content>
<DropdownMenu.Item onClick={handleCopy}>Clone</DropdownMenu.Item>
<DropdownMenu.Item onClick={handleCopy}>Duplicate</DropdownMenu.Item>
<DropdownMenu.Item color="red" onClick={handleDelete}>
Delete
</DropdownMenu.Item>
Expand Down
2 changes: 2 additions & 0 deletions src/views/Recorder/Recorder.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,14 @@ import { useListenProxyData } from '@/hooks/useListenProxyData'
import { PageHeading } from '@/components/Layout/PageHeading'
import { useRecorderStore } from '@/hooks/useRecorderStore'
import { groupProxyData } from '@/utils/groups'
import { useWindowTitle } from '@/hooks/useWindowTitle'

export function Recorder() {
const { proxyData } = useRecorderStore()
const [group, setGroup] = useState<string>('Default')
useListenProxyData(group)
const groupedProxyData = groupProxyData(proxyData)
useWindowTitle('Recorder')

return (
<>
Expand Down
5 changes: 4 additions & 1 deletion src/views/Validator/Validator.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { LogView } from '@/components/LogView'
import { WebLogView } from '@/components/WebLogView'
import { useListenProxyData } from '@/hooks/useListenProxyData'
import { useRecorderStore } from '@/hooks/useRecorderStore'
import { useWindowTitle } from '@/hooks/useWindowTitle'
import { K6Log } from '@/types'
import { groupProxyData } from '@/utils/groups'
import { Button, Flex, Heading, ScrollArea, Spinner } from '@radix-ui/themes'
Expand All @@ -13,7 +14,9 @@ export function Validator() {
const [isRunning, setIsRunning] = useState(false)
const [logs, setLogs] = useState<K6Log[]>([])
useListenProxyData()
const { proxyData, resetProxyData } = useRecorderStore()
const proxyData = useRecorderStore((store) => store.proxyData)
const resetProxyData = useRecorderStore((store) => store.resetProxyData)
useWindowTitle('Validator')

const groupedProxyData = groupProxyData(proxyData)

Expand Down
Loading