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: Simpler default file names #453

Merged
merged 5 commits into from
Feb 10, 2025
Merged

feat: Simpler default file names #453

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d9873c8
feat: Simpler default file names
going-confetti Feb 5, 2025
e883209
remove generateFileNameWithTimestamp
going-confetti Feb 5, 2025
c2153db
use writeFile directly instead of checking access first
going-confetti Feb 7, 2025
63331f5
fix regression with creating new generators
going-confetti Feb 7, 2025
a509aed
more "natural" file sorting
going-confetti Feb 7, 2025
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
25 changes: 3 additions & 22 deletions src/hooks/useCreateGenerator.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,14 @@
import { useNavigate } from 'react-router-dom'
import { useCreateGenerator } from './useCreateGenerator'
import { createNewGeneratorFile } from '@/utils/generator'
import { generateFileNameWithTimestamp } from '@/utils/file'
import { getRoutePath } from '@/routeMap'
import { useToast } from '@/store/ui/useToast'
import { beforeEach, describe, expect, it, vi } from 'vitest'
import { renderHook } from '@testing-library/react'
import { act } from 'react'
import { createGeneratorData } from '@/test/factories/generator'

vi.mock('react-router-dom', () => ({
useNavigate: vi.fn(),
}))
vi.mock('@/utils/generator', () => ({
createNewGeneratorFile: vi.fn(),
}))
vi.mock('@/utils/file', () => ({
generateFileNameWithTimestamp: vi.fn(),
}))
vi.mock('@/routeMap', () => ({
getRoutePath: vi.fn(),
}))
Expand All @@ -41,16 +32,14 @@ describe('useCreateGenerator', () => {
})

it('should navigate to the correct path on successful generator creation', async () => {
const newGenerator = createGeneratorData()
const fileName = 'test-file.json'
const routePath = '/generator/test-file.json'

vi.mocked(createNewGeneratorFile).mockReturnValue(newGenerator)
vi.mocked(generateFileNameWithTimestamp).mockReturnValue(fileName)
vi.mocked(getRoutePath).mockReturnValue(routePath)
vi.stubGlobal('studio', {
generator: {
saveGenerator: vi.fn().mockResolvedValue(fileName),
createGenerator: vi.fn().mockResolvedValue(fileName),
saveGenerator: vi.fn(),
loadGenerator: vi.fn(),
},
})
Expand All @@ -61,15 +50,7 @@ describe('useCreateGenerator', () => {
await result.current()
})

expect(createNewGeneratorFile).toHaveBeenCalled()
expect(generateFileNameWithTimestamp).toHaveBeenCalledWith(
'json',
'Generator'
)
expect(window.studio.generator.saveGenerator).toHaveBeenCalledWith(
JSON.stringify(newGenerator, null, 2),
fileName
)
expect(window.studio.generator.createGenerator).toHaveBeenCalledWith()
expect(navigate).toHaveBeenCalledWith(routePath)
})

Expand Down
8 changes: 1 addition & 7 deletions src/hooks/useCreateGenerator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
import { useCallback } from 'react'
import { useNavigate } from 'react-router-dom'

import { generateFileNameWithTimestamp } from '@/utils/file'
import { createNewGeneratorFile } from '@/utils/generator'
import { getRoutePath } from '@/routeMap'
import { useToast } from '@/store/ui/useToast'
import log from 'electron-log/renderer'
Expand All @@ -13,11 +11,7 @@ export function useCreateGenerator() {

const createTestGenerator = useCallback(async () => {
try {
const newGenerator = createNewGeneratorFile()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No need to do it in the render process

const fileName = await window.studio.generator.saveGenerator(
JSON.stringify(newGenerator, null, 2),
generateFileNameWithTimestamp('json', 'Generator')
)
const fileName = await window.studio.generator.createGenerator()

navigate(
getRoutePath('generator', { fileName: encodeURIComponent(fileName) })
Expand Down
93 changes: 74 additions & 19 deletions src/main.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,8 @@ import {
INVALID_FILENAME_CHARS,
TEMP_GENERATOR_SCRIPT_FILENAME,
} from './constants/files'
import { generateFileNameWithTimestamp } from './utils/file'
import { HarFile } from './types/har'
import { GeneratorFile } from './types/generator'
import { HarFile, HarWithOptionalResponse } from './types/har'
import { GeneratorFileData } from './types/generator'
import kill from 'tree-kill'
import find from 'find-process'
import { getLogContent, initializeLogger, openLogFolder } from './logger'
Expand All @@ -70,6 +69,7 @@ import * as Sentry from '@sentry/electron/main'
import { exhaustive } from './utils/typescript'
import { DataFilePreview } from './types/testData'
import { parseDataFile } from './utils/dataFile'
import { createNewGeneratorFile } from './utils/generator'

if (process.env.NODE_ENV !== 'development') {
// handle auto updates
Expand Down Expand Up @@ -427,11 +427,22 @@ ipcMain.handle(
)

// HAR
ipcMain.handle('har:save', async (_, data: string, prefix?: string) => {
const fileName = generateFileNameWithTimestamp('har', prefix)
await writeFile(path.join(RECORDINGS_PATH, fileName), data)
return fileName
})
ipcMain.handle(
'har:save',
async (_, data: HarWithOptionalResponse, prefix: string) => {
const fileName = await generateUniqueFileName({
directory: RECORDINGS_PATH,
ext: '.har',
prefix,
})

await writeFile(
path.join(RECORDINGS_PATH, fileName),
JSON.stringify(data, null, 4)
going-confetti marked this conversation as resolved.
Show resolved Hide resolved
)
return fileName
}
)

ipcMain.handle('har:open', async (_, fileName: string): Promise<HarFile> => {
console.info('har:open event received')
Expand Down Expand Up @@ -475,23 +486,37 @@ ipcMain.handle('har:import', async (event) => {
})

// Generator
ipcMain.handle('generator:create', async (_, recordingPath: string) => {
const generator = createNewGeneratorFile(recordingPath)
const fileName = await generateUniqueFileName({
directory: GENERATORS_PATH,
ext: '.json',
prefix: 'Generator',
})

await writeFile(
path.join(GENERATORS_PATH, fileName),
JSON.stringify(generator, null, 2)
)

return fileName
})

ipcMain.handle(
'generator:save',
async (_, generatorFile: string, fileName: string) => {
console.info('generator:save event received')

async (_, generator: GeneratorFileData, fileName: string) => {
invariant(!INVALID_FILENAME_CHARS.test(fileName), 'Invalid file name')

await writeFile(path.join(GENERATORS_PATH, fileName), generatorFile)
return fileName
await writeFile(
path.join(GENERATORS_PATH, fileName),
JSON.stringify(generator, null, 2)
)
}
)

ipcMain.handle(
'generator:open',
async (_, fileName: string): Promise<GeneratorFile> => {
console.info('generator:open event received')

async (_, fileName: string): Promise<GeneratorFileData> => {
let fileHandle: FileHandle | undefined

try {
Expand All @@ -502,9 +527,7 @@ ipcMain.handle(
// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
const generator = await JSON.parse(data)

// TODO: https://github.com/grafana/k6-studio/issues/277
// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
return { name: fileName, content: generator }
return generator
} finally {
await fileHandle?.close()
}
Expand Down Expand Up @@ -962,3 +985,35 @@ const cleanUpProxies = async () => {
kill(proc.pid)
})
}

const generateUniqueFileName = async ({
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 should be in a util file, but I'd need to create a new one, since utils/file.ts has some exports that are used in render :/

Copy link
Collaborator

@allansson allansson Feb 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This is where having the app split up based on it's entry point would be handy:

shared/
  - utils/
    - file.ts
main/
  - index.ts
render/
  - utils/
    - file.ts
  - index.ts

This would allows us to setup a no-restricted-imports rule in ESLint so that things in render/ can't import from main/ and vice versa.

directory,
prefix,
ext,
}: {
directory: string
prefix: string
ext: string
}): Promise<string> => {
const timestamp = new Date().toISOString().split('T')[0] ?? ''
const template = `${prefix ? `${prefix} - ` : ''}${timestamp}${ext}`

let uniqueFileName = template
let fileExists = await access(path.join(directory, uniqueFileName))
.then(() => true)
.catch(() => false)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason why we shouldn't use fs.existsSync here instead? It'd look semantically more appropriate as it returns a boolean value.

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 async fs functions follows the electron performance recommendations and fs.access is the recommended replacement of the deprecated async fs.exists.

Interestingly, there's the following statement about fs.exists:

Using fs.exists() to check for the existence of a file before calling fs.open(), fs.readFile(), or fs.writeFile() is not recommended. Doing so introduces a race condition, since other processes may change the file's state between the two calls. Instead, user code should open/read/write the file directly and handle the error raised if the file does not exist.

Perhaps, the pattern itself is wrong, and even access shouldn't be used here

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 what you want to do is use await fs.writeFile(path, data, { flag: 'ax+' }) (see flags) and repeat that with a new suffix until the write succeeds.

Copy link
Collaborator

Choose a reason for hiding this comment

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

IMO it's okay to use these methods in certain places (like a one-off operation, during initialization) that don't compromise the event loop.

@allansson's suggestion seems to handle both the check and the write at the same time so we can consider that approach as well.

If using fs.access, we could condering cleaning up the then/catch block if we move it to a helper function since it's been used in a few places.

const fileExists = await checkFileExsits(path.join(directory, uniqueFileName)

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 rewrote the function to use writeFile


// Start from 2 as it follows the the OS behavior for duplicate files
let counter = 2

while (fileExists) {
const { name, ext } = path.parse(template)
uniqueFileName = `${name} (${counter})${ext}`
fileExists = await access(path.join(directory, uniqueFileName))
.then(() => true)
.catch(() => false)
counter++
}

return uniqueFileName
}
21 changes: 15 additions & 6 deletions src/preload.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@
/* eslint-disable @typescript-eslint/no-unsafe-return */
import { ipcRenderer, contextBridge, IpcRendererEvent } from 'electron'
import { ProxyData, K6Log, K6Check, ProxyStatus, StudioFile } from './types'
import { HarFile } from './types/har'
import { GeneratorFile } from './types/generator'
import { HarFile, HarWithOptionalResponse } from './types/har'
import { GeneratorFileData } from './types/generator'
import { AddToastPayload } from './types/toast'
import { AppSettings } from './types/settings'
import * as Sentry from './sentry'
Expand Down Expand Up @@ -107,7 +107,10 @@ const script = {
} as const

const har = {
saveFile: (data: string, prefix?: string): Promise<string> => {
saveFile: (
data: HarWithOptionalResponse,
prefix: string
): Promise<string> => {
return ipcRenderer.invoke('har:save', data, prefix)
},
openFile: (filePath: string): Promise<HarFile> => {
Expand Down Expand Up @@ -160,10 +163,16 @@ const ui = {
} as const

const generator = {
saveGenerator: (generatorFile: string, fileName: string): Promise<string> => {
return ipcRenderer.invoke('generator:save', generatorFile, fileName)
createGenerator: (): Promise<string> => {
return ipcRenderer.invoke('generator:create')
},
saveGenerator: (
generator: GeneratorFileData,
fileName: string
): Promise<void> => {
return ipcRenderer.invoke('generator:save', generator, fileName)
},
loadGenerator: (fileName: string): Promise<GeneratorFile> => {
loadGenerator: (fileName: string): Promise<GeneratorFileData> => {
return ipcRenderer.invoke('generator:open', fileName)
},
} as const
Expand Down
5 changes: 0 additions & 5 deletions src/types/generator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,4 @@
import { z } from 'zod'
import { GeneratorFileDataSchema } from '@/schemas/generator'

export interface GeneratorFile {
name: string
content: GeneratorFileData
}

export type GeneratorFileData = z.infer<typeof GeneratorFileDataSchema>
13 changes: 0 additions & 13 deletions src/utils/file.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,19 +2,6 @@ import { getRoutePath } from '../routeMap'
import { StudioFileType } from '@/types'
import { exhaustive } from './typescript'

export function generateFileNameWithTimestamp(
extension: string,
prefix?: string
) {
const timestamp =
new Date()
.toISOString()
.replace(/:/g, '-')
.replace(/T/g, '_')
.split('.')[0] ?? ''
return `${prefix ? `${prefix} - ` : ''}${timestamp}.${extension}`
}

export function getFileNameWithoutExtension(fileName: string) {
return fileName.replace(/\.[^/.]+$/, '')
}
Expand Down
8 changes: 4 additions & 4 deletions src/utils/rules.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ export function createEmptyRule(type: TestRule['type']): TestRule {
case 'correlation':
return {
type: 'correlation',
id: self.crypto.randomUUID(),
id: crypto.randomUUID(),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Question: although it's not a big deal, should we stop storing rule ids in the generator file?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right now, I believe it's only used by the state manager (so we know each one is selected/being edited etc). It might be easier to have an id on file than setting than during runtime 🤔

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 we should store them because:

  1. They are convenient when writing the frontend, e.g. for key props.
  2. If you diff two files, you can tell whether it was one thing being changed or one thing being removed and another added.
  3. JavaScript in general has no concept of value equality when it comes to complex types, so being able to easily check for equality via an id is very handy.

Like @cristianoventura said, setting them during runtime is harder.

You'd have to traverse the entire hierarchy of objects and slap and id on the things you care about, and if you load the thing from disk again you'll get an entirely new object with new id:s.

For instance, consider checking if a file has unsaved changes. If the id:s aren't stored, we would have to strip all the ids from everything before doing any comparison.

That said, could we switch to nanoid? No need to use crypto-grade randomness and nanoids are a bit terser than UUIDs. 😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That said, could we switch to nanoid? No need to use crypto-grade randomness and nanoids are a bit terser than UUIDs. 😅

We could, but what would be the benefits of this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We could, but what would be the benefits of this?

None, really. A bit more efficient (crypto is computationally expensive), saves a few bytes of space and IMO they are easier to read than UUIDs.

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 think the last argument is the strongest one, but these ids aren't user-facing so I wouldn't make it a priority.
As for performance, in the benchmarks I found crypto.randomUUID() had much higher number of operations per second than nanoid(), but it probably doesn't make any measurable difference in the context of k6 studio

enabled: true,
extractor: {
filter: { path: '' },
Expand All @@ -23,7 +23,7 @@ export function createEmptyRule(type: TestRule['type']): TestRule {
case 'customCode':
return {
type: 'customCode',
id: self.crypto.randomUUID(),
id: crypto.randomUUID(),
enabled: true,
filter: { path: '' },
snippet: '',
Expand All @@ -32,7 +32,7 @@ export function createEmptyRule(type: TestRule['type']): TestRule {
case 'parameterization':
return {
type: 'parameterization',
id: self.crypto.randomUUID(),
id: crypto.randomUUID(),
enabled: true,
filter: { path: '' },
selector: {
Expand All @@ -46,7 +46,7 @@ export function createEmptyRule(type: TestRule['type']): TestRule {
case 'verification':
return {
type: 'verification',
id: self.crypto.randomUUID(),
id: crypto.randomUUID(),
enabled: true,
filter: { path: '' },
selector: {
Expand Down
13 changes: 6 additions & 7 deletions src/views/Generator/Generator.hooks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,7 @@ import { useParams } from 'react-router-dom'
import invariant from 'tiny-invariant'

import { useToast } from '@/store/ui/useToast'
import {
loadGeneratorFile,
loadHarFile,
writeGeneratorToFile,
} from './Generator.utils'
import { loadGeneratorFile, loadHarFile } from './Generator.utils'
import { selectGeneratorData, useGeneratorStore } from '@/store/generator'
import { GeneratorFileData } from '@/types/generator'
import { useMutation, useQuery } from '@tanstack/react-query'
Expand Down Expand Up @@ -41,7 +37,10 @@ export function useUpdateValueInGeneratorFile(fileName: string) {
return useMutation({
mutationFn: async ({ key, value }: { key: string; value: unknown }) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nothing to do with this PR really, but just an idea: instead of passing a key and value to update (which doesn't seem to be type safe?), why not use a map function:

useUpdateInFile(fileName: (generator: Generator) => {
  return {
    ...generator,
    myValue: "hello"
  }
})

const generator = await loadGeneratorFile(fileName)
await writeGeneratorToFile(fileName, { ...generator, [key]: value })
await window.studio.generator.saveGenerator(
{ ...generator, [key]: value },
fileName
)
},
})
}
Expand All @@ -51,7 +50,7 @@ export function useSaveGeneratorFile(fileName: string) {

return useMutation({
mutationFn: async (generator: GeneratorFileData) => {
await writeGeneratorToFile(fileName, generator)
await window.studio.generator.saveGenerator(generator, fileName)
await queryClient.invalidateQueries({ queryKey: ['generator', fileName] })
},

Expand Down
14 changes: 2 additions & 12 deletions src/views/Generator/Generator.utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,19 +31,9 @@ export async function exportScript(fileName: string) {
await window.studio.script.saveScript(script, fileName)
}

export const writeGeneratorToFile = (
fileName: string,
generatorData: GeneratorFileData
) => {
return window.studio.generator.saveGenerator(
JSON.stringify(generatorData, null, 2),
fileName
)
}

export const loadGeneratorFile = async (fileName: string) => {
const generatorFile = await window.studio.generator.loadGenerator(fileName)
return GeneratorFileDataSchema.parse(generatorFile.content)
const generator = await window.studio.generator.loadGenerator(fileName)
return GeneratorFileDataSchema.parse(generator)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

IMO this should be happening in main

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed.

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'll open a separate PR for this change

}

export const loadHarFile = async (fileName: string) => {
Expand Down
Loading