Add vm beta #69
Add vm beta #69
Conversation
|
Review changes with Changed Files
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR introduces VM management functionality to the CLI with feature flag support, including commands for listing VMs, managing startup scripts, and viewing logs.
- Added
src/lib/vm.tswith three core commands:vm list,vm script, andvm logsfor VM management - Implemented feature flag caching system in
src/helpers/feature-flags.tswith 24-hour expiration - Added
isFeatureEnabledfunction insrc/lib/posthog.tsto gate VM features behind 'vms' flag - Potential architecture concern: Mixed usage of Node.js and Deno APIs in
feature-flags.tscould cause compatibility issues - Security enhancement: All VM commands properly handle unauthorized access with session token expiration checks
5 file(s) reviewed, 10 comment(s)
Edit PR Review Bot Settings | Greptile
| if (!exchangeAccountId) { | ||
| const response = await fetch(await getApiUrl("me"), { | ||
| method: "GET", | ||
| headers: { | ||
| "Content-Type": "application/json", | ||
| Authorization: `Bearer ${config.auth_token}`, | ||
| }, | ||
| }); | ||
|
|
||
| const data = await response.json(); | ||
| if (data.id) { | ||
| exchangeAccountId = data.id; | ||
| saveConfig({ ...config, account_id: data.id }); | ||
| } | ||
| } |
There was a problem hiding this comment.
logic: no error handling for failed API requests - could silently fail if network error or invalid response
| if (!exchangeAccountId) { | |
| const response = await fetch(await getApiUrl("me"), { | |
| method: "GET", | |
| headers: { | |
| "Content-Type": "application/json", | |
| Authorization: `Bearer ${config.auth_token}`, | |
| }, | |
| }); | |
| const data = await response.json(); | |
| if (data.id) { | |
| exchangeAccountId = data.id; | |
| saveConfig({ ...config, account_id: data.id }); | |
| } | |
| } | |
| if (!exchangeAccountId) { | |
| try { | |
| const response = await fetch(await getApiUrl("me"), { | |
| method: "GET", | |
| headers: { | |
| "Content-Type": "application/json", | |
| Authorization: `Bearer ${config.auth_token}`, | |
| }, | |
| }); | |
| if (!response.ok) { | |
| throw new Error(`API request failed with status ${response.status}`); | |
| } | |
| const data = await response.json(); | |
| if (data.id) { | |
| exchangeAccountId = data.id; | |
| saveConfig({ ...config, account_id: data.id }); | |
| } | |
| } catch (error) { | |
| console.error('Failed to fetch account ID:', error); | |
| } | |
| } |
There was a problem hiding this comment.
I think this one might be a real concern
| } catch (error) { | ||
| console.error("boba error saving feature flags:", error); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
@JohnPhamous @Flaque one of you should rebase this branch on main, I believe this change is outdated.
| const response = await fetch(await getApiUrl("me"), { | ||
| method: "GET", | ||
| headers: { | ||
| "Content-Type": "application/json", | ||
| Authorization: `Bearer ${config.auth_token}`, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
does the OpenAPI client fail to properly generate types here? all good if so
There was a problem hiding this comment.
const client = await apiClient(); this one
| let script: string; | ||
| try { | ||
| script = readFileSync(options.file, "utf-8"); | ||
| } catch (err) { | ||
| logAndQuit(`Failed to read script file: ${err.message}`); | ||
| } |
There was a problem hiding this comment.
Fun fact JS used to have a with keyword
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/with
Not supported in strict JS/TS though
| for (const log of data.data) { | ||
| console.log(`[${log.timestamp}] ${log.message}`); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Would be nice to have this be a streaming window like fly log that you can quit out of, but prob out of scope for this PR
| declare namespace Deno { | ||
| function writeTextFile(path: string, data: string): Promise<void>; | ||
| function readTextFile(path: string): Promise<string>; | ||
| function mkdir( | ||
| path: string, | ||
| options?: { recursive?: boolean } | ||
| ): Promise<void>; | ||
| } |
There was a problem hiding this comment.
Do we need to declare a type file like this?
There was a problem hiding this comment.
Try adding deno.ns to the tsconfig in the relevant fields
https://docs.deno.com/runtime/reference/ts_config_migration/#using-the-%22lib%22-property
{
"compilerOptions": {
"lib": ["deno.ns"]
}
}
Use the Deno target with the smallest possible scope
|
Also @JohnPhamous I think you probably reformatted some files using |
No description provided.