Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a parallel Effect-based CLI implementation with new bin entry, multiple Effect layers/contexts (auth, app, project, http), new commands (init, init-without-files, login, logout, pull, push, info, claim), extensive schema/perm sync tooling, and supporting utilities and types. Changes
Sequence Diagram(s)sequenceDiagram
participant User as CLI User
participant CLI as Instant CLI (new)
participant Auth as Auth Service
participant FS as Local Config
participant Browser as Browser
User->>CLI: run `login`
CLI->>Auth: POST /dash/cli/auth/register
Auth-->>CLI: { ticket, secret }
alt headless
CLI->>User: print login URL
else interactive
CLI->>Browser: open login URL
end
CLI->>Auth: poll /dash/cli/auth/check (secret)
Auth-->>CLI: { token, email }
CLI->>FS: save token to auth config
FS-->>CLI: success
CLI->>User: print login confirmed
sequenceDiagram
participant User as CLI User
participant CLI as Instant CLI (new)
participant FS as Project FS
participant API as Instant API
participant Jobs as Indexing Jobs
User->>CLI: run `push schema`
CLI->>FS: read local schema
FS-->>CLI: local schema
CLI->>API: GET /dash/apps/{appId}/schema
API-->>CLI: remote schema
CLI->>CLI: compute diff & migration plan
CLI->>User: show plan (prompt)
alt user confirms
CLI->>API: POST /dash/apps/{appId}/schema/steps/apply
API-->>CLI: apply response (may include jobs)
CLI->>Jobs: poll indexing jobs (if present)
Jobs-->>CLI: jobs complete
CLI->>User: "Schema updated!"
else user cancels
CLI->>User: "Push cancelled"
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can scan for known vulnerabilities in your dependencies using OSV Scanner.OSV Scanner will automatically detect and report security vulnerabilities in your project's dependencies. No additional configuration is required. |
Contributor
|
View Vercel preview at instant-www-js-drewh-clief-jsv.vercel.app. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/packages/cli/src/index.js (1)
1974-1983: 🛠️ Refactor suggestion | 🟠 MajorExtract exported local-config readers out of the legacy CLI entrypoint.
Exporting these helpers from
client/packages/cli/src/index.jsmakes new code depend on a side-effect-heavy module (env load + command wiring). Move them into a neutral utility module and import from there.Also applies to: 1995-2004
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/packages/cli/src/index.js` around lines 1974 - 1983, Move the exported local-config reader helpers out of the heavy CLI entrypoint by creating a neutral utility module and re-exporting them from there: extract readLocalPermsFile and the companion reader(s) referenced near the same block (e.g., the function that returns read candidates, getPermsReadCandidates) into a new module (e.g., local-config-utils) that imports loadConfig/path/other deps directly, export the same function names, then update the original index.js to import those helpers from the new module instead of exporting them itself; ensure you preserve behavior (arguments/return shape) and update any call sites to import readLocalPermsFile and the other reader(s) from the new utility module.
🧹 Nitpick comments (5)
client/packages/cli/src/new/context/globalOpts.ts (1)
17-23: UseEffect.syncfor synchronous operations instead ofEffect.genwithout effects.This block is purely synchronous with no effects or dependencies.
Effect.syncis the semantically correct API for this use case.Suggested refactoring
export const GlobalOptsLive = Layer.effect( GlobalOpts, - Effect.gen(function* () { - return { - yes: program.optsWithGlobals()?.yes || false, - token: program.optsWithGlobals()?.token, - env: program.optsWithGlobals()?.env, - }; - }), + Effect.sync(() => { + const opts = program.optsWithGlobals(); + return { + yes: opts?.yes ?? false, + token: opts?.token, + env: opts?.env, + }; + }), );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/packages/cli/src/new/context/globalOpts.ts` around lines 17 - 23, Replace the unnecessary Effect.gen call with Effect.sync because the block is purely synchronous; locate the Effect.gen(...) expression in globalOpts.ts and change it to Effect.sync(() => ({ yes: program.optsWithGlobals()?.yes || false, token: program.optsWithGlobals()?.token, env: program.optsWithGlobals()?.env })) so it uses the synchronous API while keeping the same return shape and referencing program.optsWithGlobals().client/packages/cli/src/new/context/currentApp.ts (2)
188-203: Shadowed variableplatformat line 192.The variable
platformis declared at line 172 and shadowed inside the generator at line 192. While both reference the same service type, this can cause confusion during maintenance.♻️ Proposed fix to rename the inner variable
createEphemeralApp: async (title) => { return await Runtime.runPromise( effectRuntime, Effect.gen(function* () { - const platform = yield* PlatformApi; - const response = yield* platform.use( + const platformApi = yield* PlatformApi; + const response = yield* platformApi.use( (p) => p.createTemporaryApp({ title: title }), 'Error creating temporary app', );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/packages/cli/src/new/context/currentApp.ts` around lines 188 - 203, In createEphemeralApp the local variable platform (from yield* PlatformApi inside Effect.gen) shadows the outer platform declared earlier; rename the inner binding to a clear non-conflicting name (e.g., platformService or platformClient) and update its usages in the generator (the platform.use call and the Effect.provideService call) so the outer platform and inner service are distinct; keep the rest of the implementation (Runtime.runPromise, Effect.gen, PlatformApi, effectRuntime) unchanged.
205-213: Inconsistent use of rawfetchinstead of Effect HTTP client.
getAppsForOrguses the nativefetchAPI directly, while other methods in the same object use the Effect-based HTTP client. This bypasses the standardized error handling and is inconsistent with the codebase patterns.♻️ Suggested approach
Consider converting this to use
Runtime.runPromisewith the Effect HTTP client similar tocreateAppandcreateEphemeralApp, or document why raw fetch is preferred here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/packages/cli/src/new/context/currentApp.ts` around lines 205 - 213, getAppsForOrg currently calls the raw fetch API and bypasses the project's Effect HTTP client and unified error handling; change getAppsForOrg to use the Effect HTTP client via Runtime.runPromise (same pattern used by createApp and createEphemeralApp), build the request with the shared client and Authorization header, parse/return { apps: ... } from the client response, and propagate or convert errors consistently with the other methods so it follows the same error-handling path.client/packages/cli/src/new/context/projectInfo.ts (1)
77-86: Package manager detection runs even when not needed.When
moduleNameis already found and no installation is required, the code still detects the package manager (lines 78-86), which involves async I/O. Consider moving this detection inside theif (!moduleName && coerce)block.♻️ Proposed optimization
Move the package manager detection inside the block where it's actually needed (after line 88), since it's only used for installation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/packages/cli/src/new/context/projectInfo.ts` around lines 77 - 86, The package manager is being detected eagerly even when not needed; move the detect() call and related Effect pipeline (the code that assigns packageManager) into the branch that handles installation (the if (!moduleName && coerce) block) so it only runs when installation is required. Specifically, relocate the Effect.tryPromise(() => detect()).pipe(... mapError(...)) logic that assigns packageManager so it executes inside the if (!moduleName && coerce) branch and preserve the ProjectInfoError mapping; keep references to packageManager, detect(), moduleName and coerce so the code flow and error handling remain the same.client/packages/cli/src/new/lib/pushSchema.ts (1)
112-116: ImportRenameCommandtype or remove theas anycast by properly typing the renames array.The
as anycast on line 115 bypasses type safety. Therenameoption from Commander.js is typed asstring[] | undefined, which TypeScript cannot narrow to match theRenameCommandtype expected bybuildAutoRenameSelector.RenameCommandis a template literal type with a specific format and is available from@instantdb/platform, but it's not currently exported. Either import the type to properly annotaterenames, or validate and transform the strings to ensure they conform to the expected format at runtime.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/packages/cli/src/new/lib/pushSchema.ts` around lines 112 - 116, The renames array is being cast with `as any` to satisfy `buildAutoRenameSelector`; instead import the `RenameCommand` type from `@instantdb/platform` and type `renames` as `RenameCommand[]` (or, if the type cannot be exported, validate/transform the `rename` strings into `RenameCommand` at runtime before passing them). Update the `const renames = ...` declaration to narrow `rename: string[] | undefined` to a properly typed `RenameCommand[]` (or map/validate each entry), and then call `buildAutoRenameSelector(renames)` when `globalOpts.yes` is true; keep `resolveRenames` as the fallback. Ensure references to `rename`, `renames`, `buildAutoRenameSelector`, `ResolveRenames`/`resolveRenames`, and `GlobalOpts` are adjusted accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/packages/cli/src/index.js`:
- Around line 590-592: The guard assuming process.argv[1] exists and using '/'
splitting is brittle; update the conditional before calling program.parse to
first ensure process.argv.length > 1 and typeof process.argv[1] === 'string',
then use Node's path.basename (require('path')) on process.argv[1] to get the
executable name in a cross-platform way and compare that to 'instant-cli'; keep
the call to program.parse(process.argv) unchanged when the check passes.
In `@client/packages/cli/src/new/commands/claim.ts`:
- Around line 40-46: The catchTag handler for NotAuthedError uses Effect.gen
with a generator that has no yields (Effect.gen(function*() {
console.error(...); process.exit(1); })); replace that generator with a
synchronous Effect (e.g., Effect.sync or Effect.void) so the handler is not a
generator—call Effect.sync(() => { console.error(...); process.exit(1); }) or
Effect.void(Effect.sync(...)) as appropriate; also update the similar pattern in
the NotAuthedError handler in info.ts to the same synchronous Effect approach.
In `@client/packages/cli/src/new/commands/info.ts`:
- Around line 27-29: The catchTag error handler currently uses Effect.gen (see
the handler expression Effect.gen(function* () { console.log('Not logged in.');
})), which triggers the useYield lint rule because it's written as a generator
but contains no yield; replace Effect.gen with Effect.sync so the handler is
synchronous: change the handler to use Effect.sync(() => { console.log('Not
logged in.'); }) and ensure any imports/reference to Effect.sync are available
in the module.
In `@client/packages/cli/src/new/commands/init.ts`:
- Around line 9-11: Remove the debug log that prints potentially sensitive app
context: delete the console.log(projectInfo, appinfo) call in the init command
so CurrentApp (which may contain admin tokens/credentials) is not written to
stdout; if logging is still required for debugging, use a safe, redacted logger
that excludes secrets or log only non-sensitive fields from
ProjectInfo/CurrentApp.
In `@client/packages/cli/src/new/commands/logout.ts`:
- Around line 12-19: The logout handler's failure branch incorrectly checks
SystemError.reason === 'AlreadyExists' when calling
fs.remove(authConfigFilePath) inside Effect.match; change that check to
SystemError.reason === 'NotFound' so the branch treats a missing auth file as a
successful idempotent logout (keep the chalk.green('You were already logged
out!') message) and only call error('Failed to logout: ' + e.message) for other
errors.
In `@client/packages/cli/src/new/context/authToken.ts`:
- Around line 61-71: AuthTokenCoerceLive currently calls loginCommand with
print: true (inside the authTokenGetEffect catch for NotAuthedError), which
prints the raw token instead of persisting it; change that call to invoke
loginCommand with printing disabled (e.g., set print: false or omit the print
option) so the resulting token is saved to the config for future commands while
keeping headless: true; update the loginCommand argument in the
AuthTokenCoerceLive construction (referencing AuthTokenCoerceLive,
authTokenGetEffect, loginCommand, and the NotAuthedError handler) accordingly.
- Line 56: The line is incorrectly using yield* on NotAuthedError.make() which
returns an error instance rather than an Effect; change the return to yield/fail
the error by wrapping the result of NotAuthedError.make(...) with Effect.fail
(i.e., use Effect.fail(NotAuthedError.make(...)) where the code currently calls
NotAuthedError.make directly) so the Effect fails correctly inside the function
(update the statement in authToken.ts that references NotAuthedError.make to use
Effect.fail).
In `@client/packages/cli/src/new/context/currentApp.ts`:
- Around line 217-225: The code in getAdminToken uses
Object.keys(potentialAdminTokenEnvs) which returns string[] and causes a type
mismatch when indexing potentialAdminTokenEnvs with type; fix by iterating with
typed keys — for example cast Object.keys(...) to (keyof typeof
potentialAdminTokenEnvs)[] or use Object.entries/for (const type of
Object.keys(...) as (keyof typeof potentialAdminTokenEnvs)[]) so that envName =
potentialAdminTokenEnvs[type] is correctly typed; update the map callback
signature accordingly in getAdminToken to eliminate the TypeScript error.
In `@client/packages/cli/src/new/context/projectInfo.ts`:
- Around line 68-76: The log prints "Found undefined" when
getInstantModuleName(pkgJson) returns falsy but coerce is true; update the logic
in the projectInfo flow so the console.log(`Found
${chalk.green(moduleName)}...`) only runs when moduleName is truthy: either move
that log after the check that returns ProjectInfoError (the existing if
(!moduleName && !coerce) block) or wrap the log in an if (moduleName) guard;
reference getInstantModuleName, moduleName, coerce and the ProjectInfoError
return to locate the exact spot to change.
In `@client/packages/cli/src/new/index.ts`:
- Around line 106-108: The argument description for the pull command incorrectly
says "Which configuration to push." Update the description associated with the
'[schema|perms|all]' argument in the pull command builder in
client/packages/cli/src/new/index.ts (the string immediately following
'[schema|perms|all]') to read "Which configuration to pull. Defaults to `all`"
so it correctly references "pull" instead of "push".
- Around line 175-177: The command description for the 'claim' command is
misspelled; locate the command builder where claimDef is defined
(program.command('claim') / claimDef) and update the description string passed
to .description(...) to correct "tempoary" to "temporary" so it reads "Transfer
a temporary app into your Instant account".
- Around line 81-85: Several async command action handlers invoke
Effect.runPromise(...) without returning it (notably the .action callback that
calls loginCommand and the other async command handlers), which prevents
Commander.js from awaiting completion; update each async .action callback to
return Effect.runPromise(...) so the promise is propagated (e.g., add a leading
return before Effect.runPromise in the loginCommand handler and the other
handlers that currently call it without returning). Also fix the minor text
issues: change the pull command help text that mistakenly says "push" to "pull",
and correct the typo "tempoary" to "temporary".
In `@client/packages/cli/src/new/layer.ts`:
- Around line 52-59: AuthLayerLive currently ignores the coerce boolean — modify
AuthLayerLive to select the appropriate auth token layer based on that flag (use
AuthTokenCoerceLive when coerce is true, otherwise use AuthTokenLive or the
existing AuthTokenLayer), and import AuthTokenCoerceLive from authToken.ts; keep
the rest of the composition (InstantHttpAuthedLive, InstantHttpLayer,
BaseLayerLive, Layer.provideMerge/Layer.merge) the same so the chosen auth token
layer is merged in place of the current AuthTokenLayer.
In `@client/packages/cli/src/new/lib/handleEnv.ts`:
- Around line 15-16: detectEnvType is being invoked without project context and
the env-detection block (the logic around lines 65-87) lacks Bun detection,
which can misclassify env var names; update the call site to pass the project
directory (e.g., cwd/projectDir) into detectEnvType so it resolves based on the
target project, and extend the detection logic in detectEnvType (and the
env-detection block currently mapping potentialEnvs) to also detect Bun by
checking for '@types/bun' in package.json deps/devDeps (or other Bun-specific
markers) so potentialEnvs[envType] picks the correct name.
In `@client/packages/cli/src/new/lib/login.ts`:
- Around line 37-40: The polling retry window for the login flow is too short:
the Effect.retry call using Schedule.fixed('1 seconds') with times: 12 only
waits ~12s and should be extended to ~120s; update the Effect.retry parameters
(the Schedule.fixed interval and/or times) so the total wait is ~120 seconds
(for example change to Schedule.fixed('10 seconds') with times: 12 or keep
Schedule.fixed('1 seconds') and set times: 120) in the login polling logic where
Effect.retry and Schedule.fixed are used.
In `@client/packages/cli/src/new/lib/pullPerms.ts`:
- Around line 24-25: The code currently writes shortPermsPath (from
getPermsPathToWrite(prevPermsFile?.path)) directly which can produce a
cwd-relative write; change the write to resolve the destination against the
project/package root instead of the current working directory: compute an
absolute target by joining the package root (or repo root variable used
elsewhere in this module) with shortPermsPath before writing, and apply the same
fix for the other write operations in the 34-40 range so all writes use a
project-anchored path rather than a cwd-relative path (update any calls that use
shortPermsPath directly for fs.writeFile/rename/etc. to use the resolved
absolute path).
In `@client/packages/cli/src/new/lib/pullSchema.ts`:
- Line 60: Remove the noisy/full-schema stdout dump by deleting or disabling the
console.log(newSchemaContent) call in pullSchema.ts (the line that prints
newSchemaContent); instead, if needed for debugging, send the content to a
debug/logging facility (e.g., use an existing logger or debug flag) so the CLI
output remains stable and project internals are not leaked.
In `@client/packages/cli/src/new/lib/pushPerms.ts`:
- Around line 31-36: The pipeline handling the nullable result from
readLocalPermsFile is using Option.fromNullable inside Effect.flatMap (type
mismatch); change the pipeline so the nullable is converted into a failing
Effect using Effect.fromNullable (or Effect.fromNullable(() =>
NoPermsFileError.make({...}))) instead of Option.fromNullable. Specifically
update the expression that builds permsFile (the
Effect.tryPromise(readLocalPermsFile).pipe(...)) to call
Effect.flatMap(Effect.fromNullable(() => NoPermsFileError.make({ message: 'No
permissions file found' }))) so missing-file cases produce the intended error.
In `@client/packages/cli/src/new/lib/pushSchema.ts`:
- Around line 92-95: GetSchemaError.make is being called with the raw error
object `e`, but it expects an object shaped like { message: string }; update the
error mapping in the Effect.mapError call that follows
Effect.flatMap(HttpClientResponse.schemaBodyJson(FetchSchemaResponse)) to pass a
string message (e.g., use e?.message || String(e)) wrapped as { message: ... }
when invoking GetSchemaError.make so the constructed error has the expected
shape.
- Around line 67-69: ReadSchemaFileError is defined with no fields so calling
ReadSchemaFileError.make(e) drops the original error; update the error
type/constructor to accept and store the underlying error (or at least its
message) and then change the mapping in the localSchemaFile creation to pass the
caught error (e) or e.message into ReadSchemaFileError.make via Effect.mapError
on Effect.tryPromise(readLocalSchemaFile) so the original error details are
preserved; reference symbols: readLocalSchemaFile, Effect.tryPromise,
Effect.mapError, ReadSchemaFileError.make, and localSchemaFile.
---
Outside diff comments:
In `@client/packages/cli/src/index.js`:
- Around line 1974-1983: Move the exported local-config reader helpers out of
the heavy CLI entrypoint by creating a neutral utility module and re-exporting
them from there: extract readLocalPermsFile and the companion reader(s)
referenced near the same block (e.g., the function that returns read candidates,
getPermsReadCandidates) into a new module (e.g., local-config-utils) that
imports loadConfig/path/other deps directly, export the same function names,
then update the original index.js to import those helpers from the new module
instead of exporting them itself; ensure you preserve behavior (arguments/return
shape) and update any call sites to import readLocalPermsFile and the other
reader(s) from the new utility module.
---
Nitpick comments:
In `@client/packages/cli/src/new/context/currentApp.ts`:
- Around line 188-203: In createEphemeralApp the local variable platform (from
yield* PlatformApi inside Effect.gen) shadows the outer platform declared
earlier; rename the inner binding to a clear non-conflicting name (e.g.,
platformService or platformClient) and update its usages in the generator (the
platform.use call and the Effect.provideService call) so the outer platform and
inner service are distinct; keep the rest of the implementation
(Runtime.runPromise, Effect.gen, PlatformApi, effectRuntime) unchanged.
- Around line 205-213: getAppsForOrg currently calls the raw fetch API and
bypasses the project's Effect HTTP client and unified error handling; change
getAppsForOrg to use the Effect HTTP client via Runtime.runPromise (same pattern
used by createApp and createEphemeralApp), build the request with the shared
client and Authorization header, parse/return { apps: ... } from the client
response, and propagate or convert errors consistently with the other methods so
it follows the same error-handling path.
In `@client/packages/cli/src/new/context/globalOpts.ts`:
- Around line 17-23: Replace the unnecessary Effect.gen call with Effect.sync
because the block is purely synchronous; locate the Effect.gen(...) expression
in globalOpts.ts and change it to Effect.sync(() => ({ yes:
program.optsWithGlobals()?.yes || false, token:
program.optsWithGlobals()?.token, env: program.optsWithGlobals()?.env })) so it
uses the synchronous API while keeping the same return shape and referencing
program.optsWithGlobals().
In `@client/packages/cli/src/new/context/projectInfo.ts`:
- Around line 77-86: The package manager is being detected eagerly even when not
needed; move the detect() call and related Effect pipeline (the code that
assigns packageManager) into the branch that handles installation (the if
(!moduleName && coerce) block) so it only runs when installation is required.
Specifically, relocate the Effect.tryPromise(() => detect()).pipe(...
mapError(...)) logic that assigns packageManager so it executes inside the if
(!moduleName && coerce) branch and preserve the ProjectInfoError mapping; keep
references to packageManager, detect(), moduleName and coerce so the code flow
and error handling remain the same.
In `@client/packages/cli/src/new/lib/pushSchema.ts`:
- Around line 112-116: The renames array is being cast with `as any` to satisfy
`buildAutoRenameSelector`; instead import the `RenameCommand` type from
`@instantdb/platform` and type `renames` as `RenameCommand[]` (or, if the type
cannot be exported, validate/transform the `rename` strings into `RenameCommand`
at runtime before passing them). Update the `const renames = ...` declaration to
narrow `rename: string[] | undefined` to a properly typed `RenameCommand[]` (or
map/validate each entry), and then call `buildAutoRenameSelector(renames)` when
`globalOpts.yes` is true; keep `resolveRenames` as the fallback. Ensure
references to `rename`, `renames`, `buildAutoRenameSelector`,
`ResolveRenames`/`resolveRenames`, and `GlobalOpts` are adjusted accordingly.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
client/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (33)
client/packages/cli/package.jsonclient/packages/cli/src/index.jsclient/packages/cli/src/new/commands/claim.tsclient/packages/cli/src/new/commands/info.tsclient/packages/cli/src/new/commands/init.tsclient/packages/cli/src/new/commands/initWithoutFiles.tsclient/packages/cli/src/new/commands/login.tsclient/packages/cli/src/new/commands/logout.tsclient/packages/cli/src/new/commands/pull.tsclient/packages/cli/src/new/commands/push.tsclient/packages/cli/src/new/context/authToken.tsclient/packages/cli/src/new/context/currentApp.tsclient/packages/cli/src/new/context/globalOpts.tsclient/packages/cli/src/new/context/platformApi.tsclient/packages/cli/src/new/context/projectInfo.tsclient/packages/cli/src/new/errors.tsclient/packages/cli/src/new/index.tsclient/packages/cli/src/new/layer.tsclient/packages/cli/src/new/lib/createApp.tsclient/packages/cli/src/new/lib/handleEnv.tsclient/packages/cli/src/new/lib/http.tsclient/packages/cli/src/new/lib/login.tsclient/packages/cli/src/new/lib/pullPerms.tsclient/packages/cli/src/new/lib/pullSchema.tsclient/packages/cli/src/new/lib/pushPerms.tsclient/packages/cli/src/new/lib/pushSchema.tsclient/packages/cli/src/new/lib/ui.tsclient/packages/cli/src/new/logging.tsclient/packages/cli/src/renderSchemaPlan.tsclient/packages/cli/src/ui/index.tsclient/packages/cli/src/util/getAuthPaths.tsclient/packages/cli/tsconfig.jsonserver/src/instant/dash/ephemeral_app.clj
Comment on lines
+40
to
+46
| Effect.catchTag('NotAuthedError', () => | ||
| Effect.gen(function* () { | ||
| console.error( | ||
| `Please log in first with ${chalk.bgGray.white('instant-cli login')} to claim an app`, | ||
| ); | ||
| process.exit(1); | ||
| }), |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and read the content around lines 40-46
if [ -f "client/packages/cli/src/new/commands/claim.ts" ]; then
echo "=== File content (lines 35-50) ==="
cat -n "client/packages/cli/src/new/commands/claim.ts" | sed -n '35,50p'
else
echo "File not found"
find . -name "claim.ts" -type f 2>/dev/null | head -5
fiRepository: instantdb/instant
Length of output: 533
🏁 Script executed:
# Check for Biome configuration in the project
find . -maxdepth 3 -name "biome.json" -o -name ".biomerc*" -o -name "biome.toml" 2>/dev/nullRepository: instantdb/instant
Length of output: 73
🏁 Script executed:
# Search for Effect imports and how Effect.gen and Effect.sync are used in the codebase
rg "Effect\.(gen|sync)" -A 3 --type ts --type tsx | head -60Repository: instantdb/instant
Length of output: 88
🏁 Script executed:
# Read biome configuration
cat biome.json | head -100Repository: instantdb/instant
Length of output: 1835
🏁 Script executed:
# Search for useYield in biome config
rg "useYield" . 2>/dev/null || echo "Not found in direct search"Repository: instantdb/instant
Length of output: 87
🏁 Script executed:
# Look for Effect patterns without file type specification
rg "Effect\.(gen|sync)" -A 3 -B 1 | head -80Repository: instantdb/instant
Length of output: 5506
🏁 Script executed:
# Check the imports in the claim.ts file to understand Effect usage
head -20 "client/packages/cli/src/new/commands/claim.ts"Repository: instantdb/instant
Length of output: 693
🏁 Script executed:
# Search for Effect.sync usage to verify it's a valid alternative
rg "Effect\.sync" -B 2 -A 3 | head -60Repository: instantdb/instant
Length of output: 43
🏁 Script executed:
# Also check if there are any other Effect.gen calls without yield in the codebase (similar issue)
rg "Effect\.gen\(function\*\s*\(\)" -A 5 | grep -v "yield" | head -40Repository: instantdb/instant
Length of output: 2520
🏁 Script executed:
# Check the Effect library documentation or types by looking at imports and how sync/gen are used
rg "from.*effect" "client/packages/cli/src/new/commands/claim.ts"Repository: instantdb/instant
Length of output: 159
🏁 Script executed:
# Check package.json to see what version of effect is used
cat "client/packages/cli/package.json" | grep -A 2 -B 2 '"effect"'Repository: instantdb/instant
Length of output: 199
🏁 Script executed:
# Search for any documentation or type definitions that might show Effect.sync exists
rg "sync" "client/packages/cli/src/new" --type ts | grep -i effect | head -20Repository: instantdb/instant
Length of output: 147
🏁 Script executed:
# Let's also verify the claim about useYield being in recommended correctness rules
web_search Effect library useYield correctness rule BiomeRepository: instantdb/instant
Length of output: 111
🏁 Script executed:
# Search the web for Effect.js sync method and its relation to gen
# Let's check the effect package documentation or types
find . -name "*.d.ts" -path "*/node_modules/effect/*" 2>/dev/null | head -5Repository: instantdb/instant
Length of output: 43
🏁 Script executed:
# Since we can't directly check node_modules in this sandbox, let's look at how the code pattern should be
# Let's check if there are similar error handling patterns in the codebase
rg "Effect\.catchTag" -A 5 "client/packages/cli/src/new" | head -80Repository: instantdb/instant
Length of output: 2509
🏁 Script executed:
# Check for any comments or documentation about Effect patterns in the CLI code
rg "Effect\." "client/packages/cli/src/new" -A 2 -B 1 | grep -A 2 "process\.exit" | head -30Repository: instantdb/instant
Length of output: 43
🏁 Script executed:
# Let's check the Effect library documentation by looking at the actual package exports
# In a monorepo, there might be local effect types or wrapper files
find . -type f -name "*.ts" -o -name "*.d.ts" | xargs grep -l "Effect\.sync" 2>/dev/null | head -10Repository: instantdb/instant
Length of output: 43
🏁 Script executed:
# Check if there's a package-lock.json or yarn.lock that shows the exact effect version installed
cat "client/packages/cli/package-lock.json" 2>/dev/null | grep -A 10 '"effect":' | head -20Repository: instantdb/instant
Length of output: 43
🏁 Script executed:
# Look at the similar pattern in info.ts to see if there are comments or if it also has the same issue
cat -n "client/packages/cli/src/new/commands/info.ts" | sed -n '20,40p'Repository: instantdb/instant
Length of output: 510
Remove generator without yield from Effect.gen in catchTag handler.
The Effect.gen(function* () { ... }) block lacks any yield statements, which triggers the Biome useYield correctness lint rule (enabled via the recommended ruleset). This will block CI if linting is enforced. Replace the generator with an appropriate synchronous Effect API (e.g., Effect.sync or Effect.void).
Note: The same pattern appears in client/packages/cli/src/new/commands/info.ts (lines 27-29) and may occur elsewhere.
🧰 Tools
🪛 Biome (2.4.4)
[error] 41-46: This generator function doesn't contain yield.
(lint/correctness/useYield)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/packages/cli/src/new/commands/claim.ts` around lines 40 - 46, The
catchTag handler for NotAuthedError uses Effect.gen with a generator that has no
yields (Effect.gen(function*() { console.error(...); process.exit(1); }));
replace that generator with a synchronous Effect (e.g., Effect.sync or
Effect.void) so the handler is not a generator—call Effect.sync(() => {
console.error(...); process.exit(1); }) or Effect.void(Effect.sync(...)) as
appropriate; also update the similar pattern in the NotAuthedError handler in
info.ts to the same synchronous Effect approach.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
client/packages/cli/src/new/lib/pushSchema.ts (1)
69-71:⚠️ Potential issue | 🟠 MajorNormalize unknown errors before constructing tagged errors.
Line 70 and Line 96 pass raw
einto.make(...); Line 128 assumese.messageexists. This can produce malformed error payloads or unstable messaging.🔧 Proposed fix
const localSchemaFile = yield* Effect.tryPromise(readLocalSchemaFile).pipe( - Effect.mapError((e) => ReadSchemaFileError.make(e)), + Effect.mapError((e) => + ReadSchemaFileError.make({ + message: e instanceof Error ? e.message : String(e), + }), + ), ); @@ .pipe( Effect.flatMap(HttpClientResponse.schemaBodyJson(FetchSchemaResponse)), - Effect.mapError((e) => GetSchemaError.make(e)), + Effect.mapError((e) => + GetSchemaError.make({ + message: e instanceof Error ? e.message : String(e), + }), + ), ); @@ ).pipe( - Effect.mapError((e) => SchemaDiffError.make({ message: e.message })), + Effect.mapError((e) => + SchemaDiffError.make({ + message: e instanceof Error ? e.message : String(e), + }), + ), );#!/bin/bash # Verify tagged error payload definitions and current mapError call sites. cat -n client/packages/cli/src/new/lib/pushSchema.ts | sed -n '43,101p;120,130p' rg -n "ReadSchemaFileError\\.make\\(|GetSchemaError\\.make\\(|SchemaDiffError\\.make\\(" client/packages/cli/src/new/lib/pushSchema.ts -C2Also applies to: 95-97, 128-128
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/packages/cli/src/new/lib/pushSchema.ts` around lines 69 - 71, Normalize incoming errors before passing them to the tagged-error constructors: in the readLocalSchemaFile call and other mapError sites (where ReadSchemaFileError.make, GetSchemaError.make, and SchemaDiffError.make are used), ensure you convert unknown values to an Error-like object with a message (e.g., coerce via a helper that returns new Error(String(e)) or extracts e.message when present) and then pass that normalized Error into .make(...); update the mapError callbacks around readLocalSchemaFile, the GetSchema error mapping, and the SchemaDiff error mapping to use this normalization helper so .make(...) always receives a predictable Error with a message property.client/packages/cli/src/new/index.ts (2)
81-85:⚠️ Potential issue | 🟠 MajorReturn command promises consistently and use async parsing.
Line 81, Line 90, Line 99, Line 171, and Line 178 call
Effect.runPromise(...)without returning it; coupled with Line 316 usingprogram.parse(...), this can break async error/exit propagation in command handlers.🔧 Proposed fix pattern
- .action(async (opts) => { - Effect.runPromise( - loginCommand(opts).pipe(Effect.provide(BaseLayerLive), printRedErrors), - ); - }); + .action((opts) => { + return Effect.runPromise( + loginCommand(opts).pipe(Effect.provide(BaseLayerLive), printRedErrors), + ); + }); - .action(async () => { - Effect.runPromise(infoCommand().pipe(printRedErrors)); - }); + .action(() => { + return Effect.runPromise(infoCommand().pipe(printRedErrors)); + }); -program.parse(process.argv); +void program.parseAsync(process.argv);Commander.js current docs: for action handlers that return Promises, is `program.parseAsync()` required (vs `program.parse()`) for proper awaiting and error propagation?Also applies to: 90-94, 99-101, 171-173, 178-180, 316-316
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/packages/cli/src/new/index.ts` around lines 81 - 85, Action handlers call Effect.runPromise(...) but do not return the resulting Promise, breaking async error/exit propagation; update each command action (e.g., the loginCommand action shown) to return the Effect.runPromise(...) call so the handler returns a Promise, and ensure the CLI bootstrap uses program.parseAsync(...) (instead of program.parse(...)) so Commander awaits those returned Promises and propagates errors correctly; apply this pattern to the other handlers referenced in the review (the actions at lines around 90, 99, 171, 178) and to the program.parse call at the end.
106-112:⚠️ Potential issue | 🟡 MinorFix pull/claim command text issues.
Line 107 says “push” for the
pullcommand argument, Line 111 says “App ID to push to” underpull, and Line 177 has “tempoary” typo. These should be corrected for CLI help clarity.Also applies to: 177-177
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/packages/cli/src/new/index.ts` around lines 106 - 112, Update the CLI help text for the pull/claim command: change the option description string "Which configuration to push. Defaults to `all`" to "Which configuration to pull. Defaults to `all`" (the exact literal currently near the options block), and change the '-a --app <app-id>' description "App ID to push to. Defaults to *_INSTANT_APP_ID in .env" to "App ID to pull from. Defaults to *_INSTANT_APP_ID in .env"; also fix the typo "tempoary" to "temporary" wherever it appears (line with the misspelling). Ensure you update the exact strings shown in new/index.ts so the CLI help reflects pulling, not pushing.client/packages/cli/src/index.js (1)
590-592:⚠️ Potential issue | 🟠 MajorHarden CLI parse guard for undefined argv and cross-platform paths.
Line 590 can throw when
process.argv[1]is missing, and'/'splitting misses Windows path separators.🔧 Proposed fix
-if (process.argv[1].split('/').pop() === 'instant-cli') { +const invokedAs = path.basename(process.argv[1] ?? ''); +if ( + invokedAs === 'instant-cli' || + invokedAs === 'instant-cli.js' || + invokedAs === 'instant-cli.cmd' || + invokedAs === 'instant-cli.exe' +) { program.parse(process.argv); }#!/bin/bash # Verify current guard and existing path import. cat -n client/packages/cli/src/index.js | sed -n '14,17p;588,593p'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/packages/cli/src/index.js` around lines 590 - 592, Guard currently assumes process.argv[1] exists and uses '/' splitting which breaks on Windows; update the condition before calling program.parse to first check that process.argv[1] is a non-empty string, then use Node's path.basename (require('path').basename) to extract the executable name cross-platform and compare it to 'instant-cli'; i.e., in the index.js logic around process.argv and program.parse, replace the direct split with a safe existence check and path.basename-based comparison so program.parse(process.argv) only runs when the basename equals 'instant-cli'.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/packages/cli/src/new/commands/init.ts`:
- Around line 18-25: The init flow currently treats any failure from
readLocalSchemaFile as "file missing" and always falls back to pullSchema;
change it to distinguish a missing file (undefined) from real read/parse errors
by capturing the result as an Either and branching: call
Effect.tryPromise(readLocalSchemaFile) then convert to an Either (or use
Effect.either/Effect.match to get Left error / Right value), if Left ->
surface/return the parse/read error instead of falling back, if Right && value
is undefined -> invoke pullSchema, else (Right && value present) continue the
onSuccess path; update the match in init.ts around
readLocalSchemaFile/pullSchema so only absent files trigger pullSchema while
parse errors propagate.
---
Duplicate comments:
In `@client/packages/cli/src/index.js`:
- Around line 590-592: Guard currently assumes process.argv[1] exists and uses
'/' splitting which breaks on Windows; update the condition before calling
program.parse to first check that process.argv[1] is a non-empty string, then
use Node's path.basename (require('path').basename) to extract the executable
name cross-platform and compare it to 'instant-cli'; i.e., in the index.js logic
around process.argv and program.parse, replace the direct split with a safe
existence check and path.basename-based comparison so
program.parse(process.argv) only runs when the basename equals 'instant-cli'.
In `@client/packages/cli/src/new/index.ts`:
- Around line 81-85: Action handlers call Effect.runPromise(...) but do not
return the resulting Promise, breaking async error/exit propagation; update each
command action (e.g., the loginCommand action shown) to return the
Effect.runPromise(...) call so the handler returns a Promise, and ensure the CLI
bootstrap uses program.parseAsync(...) (instead of program.parse(...)) so
Commander awaits those returned Promises and propagates errors correctly; apply
this pattern to the other handlers referenced in the review (the actions at
lines around 90, 99, 171, 178) and to the program.parse call at the end.
- Around line 106-112: Update the CLI help text for the pull/claim command:
change the option description string "Which configuration to push. Defaults to
`all`" to "Which configuration to pull. Defaults to `all`" (the exact literal
currently near the options block), and change the '-a --app <app-id>'
description "App ID to push to. Defaults to *_INSTANT_APP_ID in .env" to "App ID
to pull from. Defaults to *_INSTANT_APP_ID in .env"; also fix the typo
"tempoary" to "temporary" wherever it appears (line with the misspelling).
Ensure you update the exact strings shown in new/index.ts so the CLI help
reflects pulling, not pushing.
In `@client/packages/cli/src/new/lib/pushSchema.ts`:
- Around line 69-71: Normalize incoming errors before passing them to the
tagged-error constructors: in the readLocalSchemaFile call and other mapError
sites (where ReadSchemaFileError.make, GetSchemaError.make, and
SchemaDiffError.make are used), ensure you convert unknown values to an
Error-like object with a message (e.g., coerce via a helper that returns new
Error(String(e)) or extracts e.message when present) and then pass that
normalized Error into .make(...); update the mapError callbacks around
readLocalSchemaFile, the GetSchema error mapping, and the SchemaDiff error
mapping to use this normalization helper so .make(...) always receives a
predictable Error with a message property.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
client/packages/cli/src/index.jsclient/packages/cli/src/new/commands/claim.tsclient/packages/cli/src/new/commands/info.tsclient/packages/cli/src/new/commands/init.tsclient/packages/cli/src/new/commands/pull.tsclient/packages/cli/src/new/index.tsclient/packages/cli/src/new/layer.tsclient/packages/cli/src/new/lib/pullSchema.tsclient/packages/cli/src/new/lib/pushSchema.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- client/packages/cli/src/new/commands/pull.ts
- client/packages/cli/src/new/layer.ts
- client/packages/cli/src/new/commands/info.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/packages/cli/src/new/lib/pushSchema.ts`:
- Around line 111-113: The mapError handlers that build SchemaValidationError
(the Effect.mapError calls that call SchemaValidationError.make with 'Invalid
Schema: ' + e.message) assume e is an Error; instead normalize the throwable
before reading .message (e.g., compute a message via a helper like
getErrorMessage or with const msg = e instanceof Error ? e.message : String(e))
and use that normalized msg in the SchemaValidationError.make call; apply the
same change to both occurrences (the Effect.mapError that references
SchemaValidationError.make around the pushSchema logic and the similar mapping
at the later occurrence).
- Around line 73-88: The current early-exit checks for missing/invalid schema in
pushSchema.ts (the blocks that check localSchemaFile and
localSchemaFile.schema?.constructor?.name !== 'InstantSchemaDef') only call
error(...) and return, causing a successful exit code; change those to return a
failing Effect (e.g., return Effect.fail(new Error('<descriptive message>')) or
the project's equivalent error effect) using the same descriptive messages used
with error(...), so callers receive a failed Effect and the process exits
non‑zero; update both the "We couldn't find your `instant.schema.ts`" branch and
the "We couldn't find your schema export" branch to return failures instead of
plain returns.
- Around line 150-157: The response is currently decoded with Schema.Any before
accessing pushRes['indexing-jobs'], which is unsafe; replace Schema.Any in the
HttpClientResponse.schemaBodyJson call with a proper schema that matches the
expected shape (e.g., an object schema with an optional "indexing-jobs" array of
the appropriate job schema) or run a validation step and handle decode failures,
then use the validated result (pushRes) when calling
waitForIndexingJobsToFinish(appId, pushRes['indexing-jobs'] || []); specifically
update the call site that uses HttpClientResponse.schemaBodyJson and the
variable pushRes so that only a correctly decoded/validated payload is passed to
waitForIndexingJobsToFinish and errors are surfaced via the Effect pipeline
instead of assuming Schema.Any.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
client/packages/cli/src/new/lib/pullSchema.tsclient/packages/cli/src/new/lib/pushSchema.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- client/packages/cli/src/new/lib/pullSchema.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (8)
client/packages/cli/src/new/index.ts (6)
99-101:⚠️ Potential issue | 🟠 MajorReturn
Effect.runPromisein info handler..action(async () => { - Effect.runPromise(infoCommand().pipe(printRedErrors)); + return Effect.runPromise(infoCommand().pipe(printRedErrors)); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/packages/cli/src/new/index.ts` around lines 99 - 101, The .action handler for the CLI info command currently invokes Effect.runPromise(infoCommand().pipe(printRedErrors)) but does not return it; update the anonymous async action callback so it returns the Effect.runPromise(...) promise (i.e., make the async handler return the Effect.runPromise call) to ensure proper promise chaining and error propagation for infoCommand and printRedErrors.
90-94:⚠️ Potential issue | 🟠 MajorReturn
Effect.runPromisein logout handler.Same issue as login - the promise is not returned.
.action(async () => { - Effect.runPromise( + return Effect.runPromise( logoutCommand().pipe(Effect.provide(BaseLayerLive), printRedErrors), ); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/packages/cli/src/new/index.ts` around lines 90 - 94, The logout handler currently calls Effect.runPromise but does not return its Promise from the .action(async () => { ... }) callback, so the CLI framework may not await completion; update the .action callback to return the Effect.runPromise(...) invocation for the logoutCommand pipeline (the call involving logoutCommand(), Effect.provide(BaseLayerLive), and printRedErrors) so the promise is propagated and the command is properly awaited.
106-108:⚠️ Potential issue | 🟡 MinorFix pull command argument description.
The description says "push" but this is the
pullcommand..argument( '[schema|perms|all]', - 'Which configuration to push. Defaults to `all`', + 'Which configuration to pull. Defaults to `all`', )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/packages/cli/src/new/index.ts` around lines 106 - 108, The CLI argument description for the pull command mistakenly refers to "push"; update the help text for the argument that shows '[schema|perms|all]' so it says "Which configuration to pull. Defaults to `all`" (modify the string passed alongside '[schema|perms|all]' in the pull command definition in new/index.ts).
171-173:⚠️ Potential issue | 🟠 MajorReturn
Effect.runPromisein push handler..action(async function (arg, inputOpts) { - Effect.runPromise(pushCommand(arg, inputOpts).pipe(printRedErrors)); + return Effect.runPromise(pushCommand(arg, inputOpts).pipe(printRedErrors)); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/packages/cli/src/new/index.ts` around lines 171 - 173, The .action handler is calling Effect.runPromise(pushCommand(arg, inputOpts).pipe(printRedErrors)) but not returning it; update the action callback to return the Effect.runPromise result so the CLI waits for pushCommand to complete. Locate the .action(async function (arg, inputOpts) { ... }) and change it to return Effect.runPromise(pushCommand(arg, inputOpts).pipe(printRedErrors)); ensuring pushCommand and printRedErrors are used as-is.
175-180:⚠️ Potential issue | 🟡 MinorReturn
Effect.runPromisein claim handler and fix typo.Two issues:
- The promise is not returned
- Line 177: "tempoary" → "temporary"
const claimDef = program .command('claim') - .description('Transfer a tempoary app into your Instant account') + .description('Transfer a temporary app into your Instant account') .action(async function () { - Effect.runPromise(claimCommand.pipe(printRedErrors)); + return Effect.runPromise(claimCommand.pipe(printRedErrors)); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/packages/cli/src/new/index.ts` around lines 175 - 180, The claim command handler doesn't return the Effect.runPromise call and the command description has a typo; update the .action handler to return Effect.runPromise(claimCommand.pipe(printRedErrors)) so the promise is propagated, and correct the .description text from "tempoary" to "temporary" to fix the spelling; reference the claim command declaration, the .action callback, Effect.runPromise, claimCommand and printRedErrors when making the changes.
81-85:⚠️ Potential issue | 🟠 MajorReturn
Effect.runPromisein async action handler for proper error propagation.The promise from
Effect.runPromiseis not returned, so Commander.js won't await completion or propagate errors correctly..action(async (opts) => { - Effect.runPromise( + return Effect.runPromise( loginCommand(opts).pipe(Effect.provide(BaseLayerLive), printRedErrors), ); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/packages/cli/src/new/index.ts` around lines 81 - 85, The async action handler currently calls Effect.runPromise(...) but does not return its Promise, so Commander won't await or propagate errors; modify the .action callback to return the Effect.runPromise(...) call (i.e., return Effect.runPromise(loginCommand(opts).pipe(Effect.provide(BaseLayerLive), printRedErrors))) so the handler awaits completion and errors propagate correctly; ensure this change is applied where .action(async (opts) => { ... }) invokes loginCommand, printRedErrors and BaseLayerLive.client/packages/cli/src/new/context/authToken.ts (2)
61-70:⚠️ Potential issue | 🟠 Major
AuthTokenCoerceLiveusesprint: truewhich prints the token instead of persisting it.When coercing login on
NotAuthedError, passingprint: truetologinCommandoutputs the token to the console rather than saving it to the config file. Subsequent commands won't have the token available.Effect.catchTag('NotAuthedError', () => loginCommand({ - print: true, + print: false, }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/packages/cli/src/new/context/authToken.ts` around lines 61 - 70, AuthTokenCoerceLive calls loginCommand with print: true which prints the token instead of persisting it; update the call inside the authTokenGetEffect catch handler to omit print: true (or explicitly set print: false / use the persist/save option) so loginCommand writes the token to config and subsequent calls to AuthToken (via authTokenGetEffect) will find the persisted token.
56-56:⚠️ Potential issue | 🔴 CriticalIncorrect usage of
yield*withNotAuthedError.make().
Schema.TaggedError.make()returns an error instance, not an Effect. Usingyield*on it doesn't properly short-circuit the generator with a failure. Wrap withEffect.fail().- return yield* NotAuthedError.make({ message: 'You are not logged in' }); + return yield* Effect.fail(NotAuthedError.make({ message: 'You are not logged in' }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/packages/cli/src/new/context/authToken.ts` at line 56, The code is incorrectly using "yield*" on NotAuthedError.make() (which returns an error instance) instead of failing the Effect; change the return to fail the effect by wrapping the error with Effect.fail(NotAuthedError.make({...})) (i.e., replace the current return yield* NotAuthedError.make({...}) with return yield* Effect.fail(NotAuthedError.make({...}))); also ensure Effect is imported where auth token logic runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/packages/cli/src/new/layer.ts`:
- Around line 70-88: WithAppLayer currently always provides AuthLayerLive and
ignores args.coerce; update the Layer.provideMerge call that supplies the auth
layer to choose between AuthLayerLive and AuthLayerCoerceLive based on
args.coerce (e.g., call AuthLayerCoerceLive() when args.coerce is true,
otherwise AuthLayerLive), keeping CurrentAppLive and ProjectInfoLive usage
unchanged so auth selection respects the coerce flag.
- Line 43: The Layer declaration AuthTokenCoerceLayer is incorrectly providing
AuthTokenLive instead of the coerced implementation; update the provider to use
AuthTokenCoerceLive (i.e., change the Layer.provide argument from AuthTokenLive
to AuthTokenCoerceLive) so AuthTokenCoerceLayer actually supplies the coerced
auth implementation and fixes AuthLayerCoerceLive behaving like AuthLayerLive.
---
Duplicate comments:
In `@client/packages/cli/src/new/context/authToken.ts`:
- Around line 61-70: AuthTokenCoerceLive calls loginCommand with print: true
which prints the token instead of persisting it; update the call inside the
authTokenGetEffect catch handler to omit print: true (or explicitly set print:
false / use the persist/save option) so loginCommand writes the token to config
and subsequent calls to AuthToken (via authTokenGetEffect) will find the
persisted token.
- Line 56: The code is incorrectly using "yield*" on NotAuthedError.make()
(which returns an error instance) instead of failing the Effect; change the
return to fail the effect by wrapping the error with
Effect.fail(NotAuthedError.make({...})) (i.e., replace the current return yield*
NotAuthedError.make({...}) with return yield*
Effect.fail(NotAuthedError.make({...}))); also ensure Effect is imported where
auth token logic runs.
In `@client/packages/cli/src/new/index.ts`:
- Around line 99-101: The .action handler for the CLI info command currently
invokes Effect.runPromise(infoCommand().pipe(printRedErrors)) but does not
return it; update the anonymous async action callback so it returns the
Effect.runPromise(...) promise (i.e., make the async handler return the
Effect.runPromise call) to ensure proper promise chaining and error propagation
for infoCommand and printRedErrors.
- Around line 90-94: The logout handler currently calls Effect.runPromise but
does not return its Promise from the .action(async () => { ... }) callback, so
the CLI framework may not await completion; update the .action callback to
return the Effect.runPromise(...) invocation for the logoutCommand pipeline (the
call involving logoutCommand(), Effect.provide(BaseLayerLive), and
printRedErrors) so the promise is propagated and the command is properly
awaited.
- Around line 106-108: The CLI argument description for the pull command
mistakenly refers to "push"; update the help text for the argument that shows
'[schema|perms|all]' so it says "Which configuration to pull. Defaults to `all`"
(modify the string passed alongside '[schema|perms|all]' in the pull command
definition in new/index.ts).
- Around line 171-173: The .action handler is calling
Effect.runPromise(pushCommand(arg, inputOpts).pipe(printRedErrors)) but not
returning it; update the action callback to return the Effect.runPromise result
so the CLI waits for pushCommand to complete. Locate the .action(async function
(arg, inputOpts) { ... }) and change it to return
Effect.runPromise(pushCommand(arg, inputOpts).pipe(printRedErrors)); ensuring
pushCommand and printRedErrors are used as-is.
- Around line 175-180: The claim command handler doesn't return the
Effect.runPromise call and the command description has a typo; update the
.action handler to return Effect.runPromise(claimCommand.pipe(printRedErrors))
so the promise is propagated, and correct the .description text from "tempoary"
to "temporary" to fix the spelling; reference the claim command declaration, the
.action callback, Effect.runPromise, claimCommand and printRedErrors when making
the changes.
- Around line 81-85: The async action handler currently calls
Effect.runPromise(...) but does not return its Promise, so Commander won't await
or propagate errors; modify the .action callback to return the
Effect.runPromise(...) call (i.e., return
Effect.runPromise(loginCommand(opts).pipe(Effect.provide(BaseLayerLive),
printRedErrors))) so the handler awaits completion and errors propagate
correctly; ensure this change is applied where .action(async (opts) => { ... })
invokes loginCommand, printRedErrors and BaseLayerLive.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
client/packages/cli/src/new/commands/info.tsclient/packages/cli/src/new/commands/initWithoutFiles.tsclient/packages/cli/src/new/context/authToken.tsclient/packages/cli/src/new/index.tsclient/packages/cli/src/new/layer.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- client/packages/cli/src/new/commands/initWithoutFiles.ts
- client/packages/cli/src/new/commands/info.ts
drew-harris
force-pushed
the
drewh/clief
branch
from
9447091 to
af52b41
Compare
drew-harris
force-pushed
the
drewh/clief
branch
from
af52b41 to
a0de7e9
Compare
- finish global opts - info command - move around init - more stuff - pull all + push perms done - a lot done + claim - start of push - wow? - clean up old version + info - parity - opts from command - remove todo - add back old pull - formatting - finish init - schema read file error - swap to coerce login - fix ts error - normaler error messages - fix type - fix logout fix coerce same error format feedback enable silent logging more logging fixes low hanging fruit explorer command track app imports add bun as potential env add solid package format for init without files allow admin token as auth fix ci tests - use new cli experimental type preservation query command instant.config.ts support fix a lot
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR rewrites 95% of the CLI in effect with organized command structure.
Behavior should be 1:1 with the current cli except for the formatting of some error messages / stack traces.
Currently adds a
new-instant-clicommand to check parity between the two. Once approved I will remove all the old code and we can switch over.Motivation
The previous version of the CLI used multiple different methods for handling all mixed together: return {ok: false}, throwing errors, returning null, process.exit. As well as had lots of duplicate functions that were simple wrappers to add error logging. This made it hard to reuse existing code for new commands without having to adapt your error handling or end up with extra print statements.
Additionally the old code relied on passing
optseverywhere to keep track of the current app/user context which made it easy to run into unexpected behavior as not every command has the same opts but lots of code that accepts and opts param is shared.The new system uses Effect for handling errors and dependencies. This allows the code used to build up the context of the cli to be much more versatile so that new commands can be added easier. We also use the Context system of effect to scope certain functions so they can only be run under certain conditions.
For example const pushSchema:
Effect<void, Error, CurrentApp | AuthToken>describes a function that returns void, can error, and requires a current app and the user to be logged in (at compile time)Example Workflow for adding new command
(This is how i would add the "login" command if it wasn't already added).
Add the definition to the src/index.ts file.
Add a new file in src/commands/login.
OptsFromCommand uses the loginDef and parses the options into a type safe variable.
Effect.provide(BaseLayerLive) provides a minimal context that includes node platform + a global reference to the global flags of the cli.
Sidenote: Anywhere in the code you can do
to access the global options for the cli.
This lets you get the current app ID from anywhere, and "colors" the current function and any function above it to require you to provide the app at the very top.
Push Example
Effect.provide(WithAppLayer) is how you provide the current app context to the inside code.
Dogfooding Check
Command List:
Claim Command
Info
Init - should push schema and perms if files exist, pull if not, should force login if not authed
Init Without Files - Should validate flags, should force login
Login - should support headless browser, should detect "double login"
Logout - should properly remove file, should detect "double logout"
Pull
Push
How To Test
Now you can use
instant-cliandnew-instant-clianywhere.