fix(tui): scroll setup agent selection#538
Open
daniel-baf wants to merge 2 commits into
Open
Conversation
📝 WalkthroughWalkthroughThe TUI setup screen gains multi-agent selection (space-bar toggling with checkboxes), paginated scrolling, and aggregated install results. The ChangesMulti-agent setup screen
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/tui/model.go`:
- Around line 254-266: The installAgentFn loop has an early return on error that
discards results from already-installed agents. Instead of immediately returning
setupInstallMsg{err: err} when installAgentFn returns an error, collect the
error and continue iterating through the remaining agents in the names slice.
Aggregate both successful installation results and per-agent errors, then return
a setupInstallMsg that includes all results that succeeded along with
information about which agents failed, ensuring users know which agents were
installed before the error occurred.
- Around line 272-278: In the multi-agent installation result synthesis (the
setupInstallMsg return statement), two issues need to be fixed: First, add a
TUIPluginEnabled field to the returned setup.Result by iterating through the
results slice and OR-ing together all TUIPluginEnabled values from each result
to preserve this flag across multi-agent installs. Second, build the Agent
string by joining the agent names extracted from the results slice instead of
using the original names slice, ensuring that only agents with successful
results are included in the summary message.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 94eafdd9-b851-421d-808e-fbba534c02a5
📒 Files selected for processing (5)
internal/tui/model.gointernal/tui/update.gointernal/tui/update_test.gointernal/tui/view.gointernal/tui/view_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/tui/model.go`:
- Around line 257-259: The setupInstallMsg being returned at line 257 in the
aggregateSetupResults condition includes both an error (err) and needsAllowlist
flag set to true. However, the update.go error handler processes msg.err first
and exits before checking msg.needsAllowlist, making the allowlist prompt
unreachable in partial-failure scenarios. To fix this, modify the return
statement in the aggregateSetupResults check to not include the error in
setupInstallMsg when needsAllowlist is true, so that the allowlist prompt
handling takes precedence over error handling. This ensures the multi-select
allowlist contract is maintained even when partial failures occur.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6ddf81cf-a0fa-4134-b606-21f33abd7104
📒 Files selected for processing (6)
internal/tui/model.gointernal/tui/model_test.gointernal/tui/update.gointernal/tui/update_test.gointernal/tui/view.gointernal/tui/view_test.go
Comment on lines
+257
to
+259
| if partial := aggregateSetupResults(results, totalFiles); partial != nil { | ||
| return setupInstallMsg{result: partial, err: err, needsAllowlist: needsAllowlist} | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Allowlist prompt is unreachable on partial-failure path.
Line 257 returns setupInstallMsg with both err and needsAllowlist, but internal/tui/update.go handles msg.err first and exits before checking msg.needsAllowlist. That means a successful claude-code install in a partial-failure batch will not surface the allowlist prompt, breaking the multi-select allowlist contract.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/tui/model.go` around lines 257 - 259, The setupInstallMsg being
returned at line 257 in the aggregateSetupResults condition includes both an
error (err) and needsAllowlist flag set to true. However, the update.go error
handler processes msg.err first and exits before checking msg.needsAllowlist,
making the allowlist prompt unreachable in partial-failure scenarios. To fix
this, modify the return statement in the aggregateSetupResults check to not
include the error in setupInstallMsg when needsAllowlist is true, so that the
allowlist prompt handling takes precedence over error handling. This ensures the
multi-select allowlist contract is maintained even when partial failures occur.
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
1 participant
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.
🔗 Linked Issue
Closes #199
🏷️ PR Type
type:bug— Bug fixtype:feature— New featuretype:docs— Documentation onlytype:refactor— Code refactoring (no behavior change)type:chore— Maintenance, dependencies, toolingtype:breaking-change— Breaking change📝 Summary
space, while keepingentercompatible with single-item installs when nothing is selected.📂 Changes
internal/tui/model.gointernal/tui/update.gointernal/tui/view.gointernal/tui/update_test.gointernal/tui/view_test.go🧪 Test Plan
GOCACHE=/tmp/engram-go-build go test ./...GOCACHE=/tmp/engram-go-build go test -tags e2e ./internal/server/...Manual verification:
engram tuiwith a temporaryHOMEand a short/shrunk terminal.j/kscrolls through 12 setup agents without printing all options at once.spacecan select multiple agents and the selected count appears.showing 7-8 of 12in a very short terminal toshowing 7-12 of 12after expanding height.Screenshot evidence from local manual test:
🤖 Automated Checks
These run automatically and all must pass before merge:
Closes #N/Fixes #N/Resolves #Nstatus:approvedlabeltype:*labelgo test ./...passesgo test -tags e2e ./internal/server/...passes✅ Contributor Checklist
Closes #199)type:*label to this PR (needs maintainer:type:bug; GitHub rejected label edit for this fork user)go test ./...go test -tags e2e ./internal/server/...Co-Authored-Bytrailers in commits💬 Notes for Reviewers
The setup screen still installs the current cursor item when no checkboxes are selected, so existing keyboard behavior remains available. Multi-install is sequential rather than batched because the current setup result UI has one installation/result state.
Maintainer action needed: add exactly one PR label,
type:bug. The fork author account could open the PR but GitHub rejectedAddLabelsToLabelablefor the upstream repo.Summary by CodeRabbit