chore: Allow multiple environment logins, new desktop authentication storage, pool configuration, swap environment support. #7714
Conversation
There was a problem hiding this comment.
Alright longer review than expected! Feels good playing with it locally.
Left lots of comments, most of them are nits and cool to move forward without addressing or creating separate issues for them. Before approving I'd like to make sure we look at @jacebrowning's feedback at #7714 (comment)
|
|
||
| const choosePoolCommand: Command = { | ||
| name: 'choose-pool', | ||
| displayName: 'Choose pool', |
There was a problem hiding this comment.
Related to the comment above about the 'pool' naming
There was a problem hiding this comment.
@franknoirot Any opinions?
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| export function generateDomainsFromBaseDomain(baseDomain: string) { | ||
| return { | ||
| API_URL: `https://api.${baseDomain}`, | ||
| SITE_URL: `https://${baseDomain}`, | ||
| WEBSOCKET_URL: `wss://api.${baseDomain}/ws/modeling/commands`, | ||
| APP_URL: `https://app.${baseDomain}`, | ||
| } | ||
| } |
There was a problem hiding this comment.
The generateDomainsFromBaseDomain function doesn't handle empty strings gracefully. When baseDomain is empty, it generates invalid URLs like https://api. which will cause API calls to fail. Consider adding a check for empty strings that returns default values or throws an appropriate error to prevent downstream failures.
| export function generateDomainsFromBaseDomain(baseDomain: string) { | |
| return { | |
| API_URL: `https://api.${baseDomain}`, | |
| SITE_URL: `https://${baseDomain}`, | |
| WEBSOCKET_URL: `wss://api.${baseDomain}/ws/modeling/commands`, | |
| APP_URL: `https://app.${baseDomain}`, | |
| } | |
| } | |
| export function generateDomainsFromBaseDomain(baseDomain: string) { | |
| if (!baseDomain) { | |
| throw new Error('Base domain cannot be empty') | |
| } | |
| return { | |
| API_URL: `https://api.${baseDomain}`, | |
| SITE_URL: `https://${baseDomain}`, | |
| WEBSOCKET_URL: `wss://api.${baseDomain}/ws/modeling/commands`, | |
| APP_URL: `https://app.${baseDomain}`, | |
| } | |
| } |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
I mean yes but no? What if I want the function to allow the empty string, it lets me check the default values of the return function.
| } else { | ||
| return new Error('Unable to logout, cannot find domain') | ||
| } |
There was a problem hiding this comment.
This function returns an Error object directly, but doesn't throw it or wrap it in a rejected Promise. This means the error won't propagate through the Promise chain and won't be caught by error handlers. Consider either:
- Throwing the error:
throw new Error('Unable to logout, cannot find domain') - Returning a rejected Promise:
return Promise.reject(new Error('Unable to logout, cannot find domain'))
This will ensure the error is properly handled by the caller's error handling mechanisms.
| } else { | |
| return new Error('Unable to logout, cannot find domain') | |
| } | |
| } else { | |
| return Promise.reject(new Error('Unable to logout, cannot find domain')) | |
| } |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
| return typeof navigator !== 'undefined' && | ||
| typeof window.navigator !== 'undefined' | ||
| ? navigator.userAgent.toLowerCase().indexOf('electron') > -1 | ||
| : true |
There was a problem hiding this comment.
The modified isDesktop() function returns true when navigator is undefined, which could lead to desktop-specific code running in non-desktop environments. Consider defaulting to false when the environment cannot be determined, as this would be safer. The current implementation assumes desktop when uncertain, which may cause unexpected behavior in server-side rendering or other environments where navigator is not available.
| return typeof navigator !== 'undefined' && | |
| typeof window.navigator !== 'undefined' | |
| ? navigator.userAgent.toLowerCase().indexOf('electron') > -1 | |
| : true | |
| return typeof navigator !== 'undefined' && | |
| typeof window.navigator !== 'undefined' | |
| ? navigator.userAgent.toLowerCase().indexOf('electron') > -1 | |
| : false |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
discussion #7668
Issue
Implementation
Environment variables
VITE_KITTYCAD_BASE_DOMAINto enumerate several URLs during runtime.VITE_KITTYCAD_API_WEBSOCKET_URLby itself otherwise use theVITE_KITTYCAD_BASE_DOMAINworkflow..env.developmentand.env.productionhave been updated to account for this.Environment related GUI
Global application commands
New login authentication keys on disk
token.txtenvironment.txtfor last environment authenticated against (desktop only)envs/<subdomain>.jsonto store token, pool, name for multiple desktop authenticationssrc/lib/desktop
@src/env