feat: persist proxy preferences #254

cristianoventura · 2024-10-15T18:13:30Z

Description

This PR adds a "Settings" page where users can set some preferences. The following was implemented:

Recorder

browser path (let users input an alternative path to where Chrome executable is located)

Proxy

mode (regular or upstream)
port
automatically find port (let k6 studio find an available port if port is busy)
upstream server and authentication

How to Test

Launch k6 studio and check that the settings JSON file was created in (/Users/username/Library/Application Support/k6 Studio)
Change the browser path and attempt to launch the recorder
Change the proxy port and check that a new process is running on that port (example: lsof -i tcp:6001)
Check that all changes are saved in the JSON file

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

Related PR(s)/Issue(s)

Resolves #236

cristianoventura · 2024-10-15T18:16:57Z

src/main.ts

+let proxyStatus: ProxyStatus = 'offline'
+export let appSettings: AppSettings


Proxy status will be tracked across its life cycle so we can observe it in the UI.

All the app configuration now lives in appSettings. This variable is dynamic and will change as settings are changed.

cristianoventura · 2024-10-15T18:18:10Z

src/main.ts

+  proxyEmitter.on('status:change', (statusName: ProxyStatus) => {
+    proxyStatus = statusName
+    mainWindow.webContents.send('proxy:status:change', statusName)
+  })


Since we already have an EventEmitter for the proxy I used it as a wrapper for changes related to the proxy status.

cristianoventura · 2024-10-15T18:19:03Z

src/main.ts


  return mainWindow
 }

 app.whenReady().then(async () => {
+  appSettings = await getSettings()


Retrieving settings is the first event when the app launches. getSettings is an upsert so it'll create a configuration file if none is found.

cristianoventura · 2024-10-15T18:20:17Z

src/main.ts

+ipcMain.handle('settings:get', async () => {
+  console.info('settings:get event received')
+  return await getSettings()
+})


Event handler to retrieve settings in the UI.

cristianoventura · 2024-10-15T18:21:28Z

src/main.ts

+ipcMain.handle('settings:save', async (event, data: AppSettings) => {
+  console.info('settings:save event received')
+
+  const browserWindow = browserWindowFromEvent(event)
+  try {
+    const diff = await saveSettings(data)
+    applySettings(diff, browserWindow)


Event handler for saving settings from the UI. saveSettings returns the diff with only settings that have been modified. applySettings is responsible for overwriting and performing specific changes.

minor nit: might make more sense to call it diff -> modifiedSettings 🤔

cristianoventura · 2024-10-15T18:22:59Z

src/proxy.ts

+    '--listen-port',
+    `${proxySettings.port}`,


The proxy should always listen on the port from the settings file.

cristianoventura · 2024-10-15T18:23:29Z

src/proxy.ts

+  if (proxyRequiresAuth(proxySettings)) {
+    const { username, password } = proxySettings.upstream
+    proxyArgs.push('--upstream-auth', `${username}:${password}`)
+  }


Appends authentication argument when upstream mode requires auth.

cristianoventura · 2024-10-15T18:25:52Z

src/main.ts

+async function applySettings(
+  diff: Partial<AppSettings>,
+  browserWindow: BrowserWindow
+) {
+  if (diff.proxy) {


This is where configuration changes when settings are saved.

same as above, renaming diff might make it more explicit/readable!

cristianoventura · 2024-10-15T18:27:47Z

src/views/Settings/ProxySettings.tsx

+      <Flex gap="2" mt="5">
+        <Text size="2">
+          Proxy status: <ProxyStatusIndicator status={proxyStatus} />
+        </Text>
+      </Flex>


This proxy status can be moved to the footer/activity bar once we implement it.

cristianoventura · 2024-10-15T18:33:55Z

src/settings.ts

+const defaultSettings: AppSettings = {
+  appVersion: version,


This file contains helpers for handling the settings JSON, including the default configuration that will be set when no file is found.

Currently, we don't support any type of migration that allows the settings file to be backward compatible in case we introduce breaking changes. I'm including appVersion here so we can keep track of the schema between releases.

src/components/Layout/ActivityBar/SettingsButton.tsx

going-confetti · 2024-10-16T13:14:19Z

src/schemas/appSettings.ts

+  })
+
+export const AppSettingsSchema = z.object({
+  appVersion: z.string(),


Should this be schemaVersion instead? I presume breaking changes will be rather rare, so we can bump the schema version when needed + add migration logic

going-confetti · 2024-10-16T13:29:03Z

src/schemas/appSettings.ts

+  .object({
+    mode: z.enum(['regular', 'upstream']),
+    port: z
+      .number({ message: 'Port number is required' })
+      .int()
+      .min(1)
+      .max(65535, { message: 'Port number must be between 1 and 65535' }),
+    findPort: z.boolean(),
+    upstream: z.object({
+      url: z.string().url({ message: 'Invalid URL' }).or(z.literal('')),
+      requireAuth: z.boolean(),
+      username: z.string().optional(),
+      password: z.string().optional(),
+    }),
+  })


Not sure about storing the upstream URL in settings if the mode is set to `regular'. Would a discriminated union work better here?
TS example:

interface RegularModeSettings { mode: 'regular' port: number findPort: boolean } interface UpstreamModeSettings { mode: 'upstream' port: number findPort: boolean url: string requireAuth: boolean username: string password: string } type ProxySettings = RegularModeSettings | UpstreamModeSettings

There

The discriminated union doesn't seem compatible with a ZodEffect (the result of a refinement) 😢 Also, IMO it would be better if we preserved the structure of the schema regardless of the values.

As a user, I think it would be nice if the form remembered my previous configuration as well in case I switch back from regular to upstream in the future.

What are your thoughts?

I don't think we plan on making app settings shareable, so it's probably not too important, but I think of a setting file as a representation of the current app settings, so having stuff that doesn't affect the app there seems odd to me.

Remembering previous choices may or may not be nice, depending on the setting IMO. For example, if I switch between two different configs often, then it makes a lot of sense, but if it's a setting I change once a year or so, then I'd find it confusing.

For example, if I switch between two different configs often, then it makes a lot of sense

Although in this case, maybe the UX would need to be different: let the user create multiple proxy configurations and add a way to choose the active one in the recorder

As a user, I think it would be nice if the form remembered my previous configuration as well in case I switch back from regular to upstream in the future.

I think it would make sense for the current session but outside of that what going-confetti mentioned seems like a better solution if it's something we want to add 🤔

The discriminated union doesn't seem compatible with a ZodEffect (the result of a refinement)

Do we have any alternative ? I also think that a discriminatedUnion would be perfect here to keep the file clean and concise 🤔

I ended up getting a discriminatedUnion to work. Now we're not persistent upstream configuration if proxy mode is regular. Same with the browser path when it's checked to detect it automatically!

going-confetti · 2024-10-16T13:30:08Z

src/schemas/appSettings.ts

+
+export const RecorderSettingsSchema = z
+  .object({
+    detectBrowserPath: z.boolean(),


Similar to the other comment: if detectBrowserPath is true, there's no need to store browserPath

Co-authored-by: Uladzimir Dzmitračkoŭ <[email protected]>

…-proxy-preferences

Llandy3d · 2024-10-17T10:16:00Z

src/main.ts

+ipcMain.handle('settings:save', async (event, data: AppSettings) => {
+  console.info('settings:save event received')
+
+  const browserWindow = browserWindowFromEvent(event)
+  try {
+    const diff = await saveSettings(data)
+    applySettings(diff, browserWindow)


minor nit: might make more sense to call it diff -> modifiedSettings 🤔

Llandy3d · 2024-10-17T10:17:22Z

src/main.ts

+async function applySettings(
+  diff: Partial<AppSettings>,
+  browserWindow: BrowserWindow
+) {
+  if (diff.proxy) {


same as above, renaming diff might make it more explicit/readable!

Llandy3d · 2024-10-17T10:33:06Z

src/schemas/appSettings.ts

+    findPort: z.boolean(),
+    upstream: z.object({
+      url: z.string().url({ message: 'Invalid URL' }).or(z.literal('')),
+      requireAuth: z.boolean(),


small nit:

findPort -> automaticallyFindOpenPort or automaticallyFindPort (makes it more explicit what it does)

requireAuth -> requiresAuth (maybe ignore this one but it seemed like it made more sense since the "proxy" requires x)

Llandy3d · 2024-10-17T10:37:51Z

src/schemas/appSettings.ts

+  .object({
+    mode: z.enum(['regular', 'upstream']),
+    port: z
+      .number({ message: 'Port number is required' })
+      .int()
+      .min(1)
+      .max(65535, { message: 'Port number must be between 1 and 65535' }),
+    findPort: z.boolean(),
+    upstream: z.object({
+      url: z.string().url({ message: 'Invalid URL' }).or(z.literal('')),
+      requireAuth: z.boolean(),
+      username: z.string().optional(),
+      password: z.string().optional(),
+    }),
+  })


As a user, I think it would be nice if the form remembered my previous configuration as well in case I switch back from regular to upstream in the future.

I think it would make sense for the current session but outside of that what going-confetti mentioned seems like a better solution if it's something we want to add 🤔

The discriminated union doesn't seem compatible with a ZodEffect (the result of a refinement)

Do we have any alternative ? I also think that a discriminatedUnion would be perfect here to keep the file clean and concise 🤔

Llandy3d · 2024-10-17T10:40:44Z

src/schemas/appSettings.ts

+export type AppSettings = z.infer<typeof AppSettingsSchema>
+export type ProxySettings = z.infer<typeof ProxySettingsSchema>


nit: for the other types we have created a src/types/filename.ts specifically for types, maybe we should do the same here for consistency ? 🤔

going-confetti

LGTM

cristianoventura · 2024-10-17T17:38:53Z

@going-confetti @Llandy3d I made changes to the PR that include:

not storing upstream settings when mode is regular
not storing browser path when detectBrowserPath is true
moved the settings page to a dialog
implemented a file picker for the browser path

Screen.Recording.2024-10-17.at.1.28.55.PM.mov

Llandy3d

🚀 🚀 🚀

Llandy3d · 2024-10-18T00:34:30Z

src/schemas/appSettings.ts

+export const RegularProxySettingsSchema = z.object({
+  mode: z.literal('regular'),
+  port: z
+    .number({ message: 'Port number is required' })
+    .int()
+    .min(1)
+    .max(65535, { message: 'Port number must be between 1 and 65535' }),
+  automaticallyFindPort: z.boolean(),
+})
+
+export const UpstreamProxySettingsSchema = RegularProxySettingsSchema.extend({
+  mode: z.literal('upstream'),
+  url: z.string().url({ message: 'Invalid URL' }).or(z.literal('')),
+  requiresAuth: z.boolean(),
+  username: z.string().optional(),
+  password: z.string().optional(),
+})


small nit: I would have probably created a BaseProxySettingsSchema to be shared between all the proxy modes and then have each of them extend that since defining the port is a common setting 🤔 (but it does not matter much here and can be a future refactor 🙇 )

cristianoventura added 18 commits October 10, 2024 15:22

feat: settings file and schema

a3f98e5

feat: allow proxy to run in different modes

15cd769

feat: add base settings view and route

79f83e0

feat: add proxy settings page

fbe16ad

fix: upstream server validation

27e6aec

feat: show proxy status in settings page

c3041c9

feat: ability to get current proxy status in ui

003d90d

feat: toast on save settings

4a8abc9

feat: clean up proxy status

a3bd445

feat: add border to status indicator

7fe6bbd

feat: loading state for settings form

89006cf

feat: add hint to port number field

27d969f

feat: add support for authentication in upstream mode

d79b71c

refactor: extract proxy settings to a separate component

d027731

feat: add support to configure browser location

449af82

feat: add hint for proxy mode

9f60d40

fix: proxy emitter error when app is restored

151579c

fix: remove unused evevnt

c985e7e

cristianoventura self-assigned this Oct 15, 2024

cristianoventura commented Oct 15, 2024

View reviewed changes

fix: reset form state when settings saved

7e7d80e

cristianoventura marked this pull request as ready for review October 15, 2024 18:55

cristianoventura requested a review from a team as a code owner October 15, 2024 18:55

cristianoventura requested review from going-confetti and Llandy3d and removed request for a team October 15, 2024 18:55

going-confetti reviewed Oct 16, 2024

View reviewed changes

cristianoventura and others added 3 commits October 16, 2024 12:05

feat: improve UX with radio button and hints

3228c08

Update src/components/Layout/ActivityBar/SettingsButton.tsx

8823896

Co-authored-by: Uladzimir Dzmitračkoŭ <[email protected]>

feat: schema version

04b6613

cristianoventura added 3 commits October 16, 2024 13:23

feat: hints on proxy mode

d9b5c32

Merge branch 'main' of github.com:grafana/k6-studio into feat/persist…

460ffda

…-proxy-preferences

feat: scrollarea in settings page

c1add15

Llandy3d reviewed Oct 17, 2024

View reviewed changes

cristianoventura added 5 commits October 17, 2024 10:05

refactor: extract types and rename variables

01836c4

feat: dont save settings that are not being used

748ffdc

refactor: rename requireAuth

a26e00c

feat: settings dialog

081503e

feat: select browser executable dialog

b19d39b

going-confetti self-requested a review October 17, 2024 17:37

going-confetti previously approved these changes Oct 17, 2024

View reviewed changes

fix: dirty form after file selection

f38dd73

cristianoventura dismissed going-confetti’s stale review via f38dd73 October 17, 2024 17:59

Llandy3d approved these changes Oct 18, 2024

View reviewed changes

going-confetti self-requested a review October 18, 2024 08:30

going-confetti approved these changes Oct 18, 2024

View reviewed changes

cristianoventura merged commit a622a55 into main Oct 18, 2024
2 checks passed

cristianoventura deleted the feat/persist-proxy-preferences branch October 18, 2024 12:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: persist proxy preferences #254

feat: persist proxy preferences #254

cristianoventura commented Oct 15, 2024

cristianoventura Oct 15, 2024

cristianoventura Oct 15, 2024

cristianoventura Oct 15, 2024

cristianoventura Oct 15, 2024

cristianoventura Oct 15, 2024

Llandy3d Oct 17, 2024

cristianoventura Oct 15, 2024

cristianoventura Oct 15, 2024

cristianoventura Oct 15, 2024

Llandy3d Oct 17, 2024

cristianoventura Oct 15, 2024

cristianoventura Oct 15, 2024

going-confetti Oct 16, 2024

going-confetti Oct 16, 2024

cristianoventura Oct 16, 2024

going-confetti Oct 17, 2024

going-confetti Oct 17, 2024

Llandy3d Oct 17, 2024

cristianoventura Oct 17, 2024

going-confetti Oct 16, 2024

Llandy3d Oct 17, 2024

Llandy3d Oct 17, 2024

Llandy3d Oct 17, 2024

Llandy3d Oct 17, 2024

Llandy3d Oct 17, 2024

going-confetti left a comment

cristianoventura commented Oct 17, 2024

Llandy3d left a comment

Llandy3d Oct 18, 2024

		let proxyStatus: ProxyStatus = 'offline'
		export let appSettings: AppSettings

		export type AppSettings = z.infer<typeof AppSettingsSchema>
		export type ProxySettings = z.infer<typeof ProxySettingsSchema>

feat: persist proxy preferences #254

feat: persist proxy preferences #254

Conversation

cristianoventura commented Oct 15, 2024

Description

Recorder

Proxy

How to Test

Checklist

Screenshots (if appropriate):

Related PR(s)/Issue(s)

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

going-confetti left a comment

Choose a reason for hiding this comment

cristianoventura commented Oct 17, 2024

Llandy3d left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment