Skip to content

Polish/browser fallback and install docs#1

Open
0xBreadguy wants to merge 3 commits into
megaeth-labs:mainfrom
crumb-trail:polish/browser-fallback-and-install-docs
Open

Polish/browser fallback and install docs#1
0xBreadguy wants to merge 3 commits into
megaeth-labs:mainfrom
crumb-trail:polish/browser-fallback-and-install-docs

Conversation

@0xBreadguy

Copy link
Copy Markdown
Collaborator

No description provided.

@0xBreadguy 0xBreadguy requested a review from deva-arun as a code owner June 11, 2026 23:07
@deva-arun

Copy link
Copy Markdown
Collaborator

The browser fallback behavior is useful, but src/auth/loopback.ts should not write directly to process.stderr. That layer should stay transport/auth-focused and not own terminal UX.

We already have command-level output plumbing via makeBrowserOpener() / injected stderr, which keeps JSON/terse output predictable and makes tests less global-stateful. Can we move fallback URL rendering into the makeBrowserOpener() callback and have openSystemBrowser() return false on open failure instead?

Also please update the test to assert against injected stderr and avoid monkey-patching global process.stderr.

Finally, can you rebase your changes as a single commit off main?


To install Node.js 22:
• Using nvm: nvm install 22
• Using apt: curl -fsSL https://deb.nodesource.com/setup_22.x | sudo -E bash - && sudo apt-get install -y nodejs"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea, can we make this cross-platform guidance?


  error "Node.js >= $required_node_major is required, but you have Node.js $(node -v 2>/dev/null || echo unknown).

  Install Node.js 22, then rerun the installer.

  Options:
    nvm:      nvm install 22
    Homebrew: brew install node
    Ubuntu:   curl -fsSL https://deb.nodesource.com/setup_22.x | sudo -E bash - && sudo apt-get install -y nodejs"

Comment thread src/auth/loopback.ts
Waiting for approval...
`,
);
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see my comment on the PR

Comment thread README.md

## Uninstall

Remove the CLI, installed releases, and the wrapper script:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you replace the existing uninstall guidance in # Logout and Uninstall with this? Also change the instructions to pipe to bash ( | bash)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants