-
Notifications
You must be signed in to change notification settings - Fork 7
Onboard nudges on next steps. Changed shell completion failure from warn to ohai so it doesn't look like installation failed. #454
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…arn to ohai so it doesn't look like installation failed.
WalkthroughAdds onboarding UX: a new showAuthNextSteps function displays a post-auth banner linking to docs; it’s invoked after successful login and signup. The install script’s completion and post-install messages are revised, with added zsh checks and expanded next-step guidance. No exported API changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI
participant AuthService as Auth Service
participant TUI as Onboarding UI
rect rgba(230,245,255,0.6)
User ->> CLI: agentuity auth login/signup
CLI ->> AuthService: Authenticate (credentials / device flow)
AuthService -->> CLI: Success / token
CLI ->> User: "Successfully logged in/signed up"
end
rect rgba(235,255,235,0.6)
note over CLI,TUI: New step
CLI ->> TUI: showAuthNextSteps()
TUI -->> User: Display banner with next commands and docs link
end
sequenceDiagram
autonumber
actor User
participant Installer as install.sh
participant Shell as bash/zsh env
User ->> Installer: Run installer
Installer ->> Shell: Place binary, update PATH
alt zsh installed and writable
Installer ->> Shell: Install zsh completions
Installer ->> User: Success + next steps (expanded)
else zsh missing/dir not writable
Installer ->> User: Non-fatal note (ohai/debug), skip zsh completion
end
Installer ->> User: Help prompt and docs link
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
install.sh (1)
365-368: Fix: fish shell PATH update writes invalid syntaxWriting export PATH into config.fish breaks fish. Use fish syntax.
Apply this diff:
- if ! grep -q "$INSTALL_PATH" "$SHELL_CONFIG"; then - printf "export PATH=\"\$PATH:%s\"\n" "$INSTALL_PATH" >> "$SHELL_CONFIG" - ohai "Added $INSTALL_PATH to PATH in $SHELL_CONFIG" - fi + if ! grep -q "$INSTALL_PATH" "$SHELL_CONFIG"; then + if echo "$SHELL" | grep -q "fish"; then + printf 'set -gx PATH $PATH "%s"\n' "$INSTALL_PATH" >> "$SHELL_CONFIG" + else + printf 'export PATH="$PATH:%s"\n' "$INSTALL_PATH" >> "$SHELL_CONFIG" + fi + ohai "Added $INSTALL_PATH to PATH in $SHELL_CONFIG" + fiOptional follow-up: tailor the post-install “source …rc” hint by shell (bash/zsh/fish) to avoid confusing fish users.
🧹 Nitpick comments (1)
install.sh (1)
384-401: Good: demoting completion write issues from warn→ohai. Add Homebrew prefix paths and Mac manual stepsOn Apple Silicon, completion dirs are usually under /opt/homebrew. Also mirror the helpful manual instructions you provided on Linux for macOS.
Apply this diff to handle both Homebrew prefixes and add Mac manual instructions:
- if [ -d "/usr/local/etc/bash_completion.d" ]; then - BASH_COMPLETION_DIR="/usr/local/etc/bash_completion.d" - if [ -w "$BASH_COMPLETION_DIR" ]; then - ohai "Generating bash completion script..." - "$INSTALL_PATH/agentuity" completion bash > "$BASH_COMPLETION_DIR/agentuity" - ohai "Bash completion installed to $BASH_COMPLETION_DIR/agentuity" - else - ohai "No write permission to $BASH_COMPLETION_DIR. Skipping bash completion installation." - fi - fi + for BASH_COMPLETION_DIR in "/opt/homebrew/etc/bash_completion.d" "/usr/local/etc/bash_completion.d"; do + [ -d "$BASH_COMPLETION_DIR" ] || continue + if [ -w "$BASH_COMPLETION_DIR" ]; then + ohai "Generating bash completion script..." + "$INSTALL_PATH/agentuity" completion bash > "$BASH_COMPLETION_DIR/agentuity" + ohai "Bash completion installed to $BASH_COMPLETION_DIR/agentuity" + else + ohai "No write permission to $BASH_COMPLETION_DIR. Skipping bash completion installation." + ohai "You can manually install bash completion with:" + echo " $INSTALL_PATH/agentuity completion bash > ~/.bash_completion" + fi + done @@ - if command -v zsh >/dev/null 2>&1 && [ -d "/usr/local/share/zsh/site-functions" ]; then - ZSH_COMPLETION_DIR="/usr/local/share/zsh/site-functions" - if [ -w "$ZSH_COMPLETION_DIR" ]; then - ohai "Generating zsh completion script..." - "$INSTALL_PATH/agentuity" completion zsh > "$ZSH_COMPLETION_DIR/_agentuity" - ohai "Zsh completion installed to $ZSH_COMPLETION_DIR/_agentuity" - else - ohai "No write permission to $ZSH_COMPLETION_DIR. Skipping zsh completion installation." - fi - elif ! command -v zsh >/dev/null 2>&1; then - # Only skip silently if zsh is not installed (avoid unnecessary warnings) - debug "Zsh not found, skipping zsh completion installation" - fi + if command -v zsh >/dev/null 2>&1; then + for ZSH_COMPLETION_DIR in "/opt/homebrew/share/zsh/site-functions" "/usr/local/share/zsh/site-functions"; do + [ -d "$ZSH_COMPLETION_DIR" ] || continue + if [ -w "$ZSH_COMPLETION_DIR" ]; then + ohai "Generating zsh completion script..." + "$INSTALL_PATH/agentuity" completion zsh > "$ZSH_COMPLETION_DIR/_agentuity" + ohai "Zsh completion installed to $ZSH_COMPLETION_DIR/_agentuity" + else + ohai "No write permission to $ZSH_COMPLETION_DIR. Skipping zsh completion installation." + ohai "You can manually install zsh completion with:" + echo " mkdir -p ~/.zsh/completion" + echo " $INSTALL_PATH/agentuity completion zsh > ~/.zsh/completion/_agentuity" + echo " echo 'fpath=(~/.zsh/completion \$fpath)' >> ~/.zshrc" + echo " echo 'autoload -U compinit && compinit' >> ~/.zshrc" + fi + done + else + # Only skip silently if zsh is not installed (avoid unnecessary warnings) + debug "Zsh not found, skipping zsh completion installation" + fiAlso applies to: 412-436
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/auth.go(2 hunks)cmd/onboarding.go(1 hunks)install.sh(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and Test (macos-latest)
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
cmd/auth.go (1)
115-116: LGTM: Showing next steps right after success improves onboarding flowThe placement after the success banner is good and non-intrusive.
Please confirm we also show these steps after any future auth paths (e.g., SSO) for consistency.
Also applies to: 223-224
install.sh (1)
442-450: LGTM: Clear “Next steps” matches the onboarding bannerConcise, actionable, and consistent with the new auth flow.
Summary by CodeRabbit