feat: Simpler default file names #453

going-confetti · 2025-02-05T10:23:03Z

Description

Make auto-generated file names shorter and reduce noise, by removing the timestamp and using optional increment at the end.
Remove GeneratorFile as it's become redundant
Stringify HAR/Generator data in the main process

How to Test

Verify that the following works as before (apart from the new filenames):

Create an new generator, then do it again on the same date
Create a few new recordings with the same starting URL
Create a new recording without a starting URL
Try re-configuring a generator and saving changes

going-confetti

Self-review

going-confetti · 2025-02-05T10:42:22Z

src/hooks/useCreateGenerator.ts

@@ -13,11 +11,7 @@ export function useCreateGenerator() {

  const createTestGenerator = useCallback(async () => {
    try {
-      const newGenerator = createNewGeneratorFile()


No need to do it in the render process

src/main.ts

going-confetti · 2025-02-05T10:45:59Z

src/main.ts

@@ -962,3 +985,35 @@ const cleanUpProxies = async () => {
    kill(proc.pid)
  })
 }
+
+const generateUniqueFileName = async ({


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

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.

going-confetti · 2025-02-05T10:48:45Z

src/utils/rules.ts

@@ -5,7 +5,7 @@ export function createEmptyRule(type: TestRule['type']): TestRule {
    case 'correlation':
      return {
        type: 'correlation',
-        id: self.crypto.randomUUID(),
+        id: crypto.randomUUID(),


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

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 🤔

I think we should store them because:

They are convenient when writing the frontend, e.g. for key props.

If you diff two files, you can tell whether it was one thing being changed or one thing being removed and another added.

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. 😅

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?

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.

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

going-confetti · 2025-02-05T10:49:11Z

src/views/Generator/Generator.utils.ts

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


IMO this should be happening in main

I'll open a separate PR for this change

going-confetti · 2025-02-05T10:50:04Z

src/views/Recorder/Recorder.tsx

-        JSON.stringify(har, null, 4),
-        prefix
-      )
+      const prefix = getHostNameFromURL(startUrl) ?? 'Recording'


Here's another change: if no starting URL is provided, the name will follow the Recording - YYYY-MM-DD pattern

cristianoventura

It works as expected! 🚀

I left a comment for clarification before approving.

src/main.ts

cristianoventura · 2025-02-05T17:27:43Z

src/main.ts

+  let fileExists = await access(path.join(directory, uniqueFileName))
+    .then(() => true)
+    .catch(() => false)


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.

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

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.

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)

I rewrote the function to use writeFile

cristianoventura · 2025-02-05T17:31:22Z

src/utils/rules.ts

@@ -5,7 +5,7 @@ export function createEmptyRule(type: TestRule['type']): TestRule {
    case 'correlation':
      return {
        type: 'correlation',
-        id: self.crypto.randomUUID(),
+        id: crypto.randomUUID(),


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 🤔

allansson · 2025-02-06T13:38:37Z

src/views/Generator/Generator.hooks.ts

@@ -41,7 +37,10 @@ export function useUpdateValueInGeneratorFile(fileName: string) {
  return useMutation({
    mutationFn: async ({ key, value }: { key: string; value: unknown }) => {


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" } })

cristianoventura · 2025-02-06T16:31:15Z

src/main.ts

+  let fileExists = await access(path.join(directory, uniqueFileName))
+    .then(() => true)
+    .catch(() => false)


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)

Llandy3d

LGTM! 🙌

allansson

Really like this change. Remove much of the clutter in the sidebar and it overall just feel more organized now.

Great job! 💯

going-confetti added 2 commits February 5, 2025 11:22

feat: Simpler default file names

d9873c8

remove generateFileNameWithTimestamp

e883209

going-confetti commented Feb 5, 2025

View reviewed changes

going-confetti marked this pull request as ready for review February 5, 2025 10:50

going-confetti requested a review from a team as a code owner February 5, 2025 10:50

going-confetti requested review from cristianoventura, Llandy3d and allansson February 5, 2025 10:50

cristianoventura reviewed Feb 5, 2025

View reviewed changes

allansson reviewed Feb 6, 2025

View reviewed changes

cristianoventura previously approved these changes Feb 6, 2025

View reviewed changes

use writeFile directly instead of checking access first

c2153db

going-confetti dismissed cristianoventura’s stale review via c2153db February 7, 2025 09:25

going-confetti requested review from allansson and cristianoventura February 7, 2025 09:34

fix regression with creating new generators

63331f5

Llandy3d previously approved these changes Feb 7, 2025

View reviewed changes

more "natural" file sorting

a509aed

going-confetti dismissed Llandy3d’s stale review via a509aed February 7, 2025 15:01

Llandy3d approved these changes Feb 7, 2025

View reviewed changes

allansson approved these changes Feb 10, 2025

View reviewed changes

going-confetti merged commit 445202e into main Feb 10, 2025
3 checks passed

going-confetti deleted the feat/simpler-autogenerated-names branch February 10, 2025 10:06

This was referenced Feb 7, 2025

chore(main): release 0.13.0 #432

Merged

chore(main): release 0.14.0 #489

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Simpler default file names #453

feat: Simpler default file names #453

going-confetti commented Feb 5, 2025 •

edited

Loading

going-confetti left a comment

going-confetti Feb 5, 2025

going-confetti Feb 5, 2025

allansson Feb 6, 2025 •

edited

Loading

going-confetti Feb 5, 2025

cristianoventura Feb 5, 2025

allansson Feb 6, 2025

going-confetti Feb 7, 2025

allansson Feb 7, 2025

going-confetti Feb 7, 2025

going-confetti Feb 5, 2025

allansson Feb 6, 2025

going-confetti Feb 6, 2025

going-confetti Feb 5, 2025

cristianoventura left a comment

cristianoventura Feb 5, 2025

going-confetti Feb 6, 2025

allansson Feb 6, 2025

cristianoventura Feb 6, 2025

going-confetti Feb 7, 2025

cristianoventura Feb 5, 2025

allansson Feb 6, 2025

cristianoventura Feb 6, 2025

Llandy3d left a comment

allansson left a comment

		@@ -41,7 +37,10 @@ export function useUpdateValueInGeneratorFile(fileName: string) {
		return useMutation({
		mutationFn: async ({ key, value }: { key: string; value: unknown }) => {

feat: Simpler default file names #453

feat: Simpler default file names #453

Conversation

going-confetti commented Feb 5, 2025 • edited Loading

Description

How to Test

going-confetti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allansson Feb 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cristianoventura left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Llandy3d left a comment

Choose a reason for hiding this comment

allansson left a comment

Choose a reason for hiding this comment

going-confetti commented Feb 5, 2025 •

edited

Loading

allansson Feb 6, 2025 •

edited

Loading