Phase 2: bubbletea review screen for setup import - #129
Merged
Conversation
Reviewer's GuideImplements a Bubbletea-based TUI review screen for setup import, adds detection metadata for auto-resolved items, and wires decisions into the existing import pipeline while preserving non-TTY behavior, plus dependency and test coverage updates. Sequence diagram for setup import review TUI flowsequenceDiagram
actor User
participant runSetup
participant runDetection
participant runReviewImport
participant applyReviewDecisions
User->>runSetup: runSetup
runSetup->>runSetup: reviewTTYAvailable
alt TTY available
runSetup->>runDetection: runDetection
runSetup->>runReviewImport: runReviewImport
runReviewImport->>runReviewImport: tea.NewProgram
runReviewImport->>runReviewImport: Program.Run
alt user applies
runReviewImport->>applyReviewDecisions: applyReviewDecisions
applyReviewDecisions->>applyReviewDecisions: copyDirMissing / importRoleCandidate / upsertCanonicalMCPServer
else user aborts
runReviewImport->>runSetup: errReviewAborted
end
else non-TTY
runSetup->>applyReviewDecisions: importNativeContent
end
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
newReviewModel, consider deriving theresolvedcount fromlen(detection.Resolved)instead ofdetection.AutoResolvedso the model stays consistent if the way auto-resolved items are tracked changes in the future. - In
runReviewImport, the bubbletea program is hard-wired to the real stdio while the rest of setup usesstreams; if you ever need to support alternative IO (e.g., testing harnesses or wrapped streams) it may be cleaner to pass a customtea.ProgramOptionswired tostreamsinstead ofos.Stdin/os.Stdout.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `newReviewModel`, consider deriving the `resolved` count from `len(detection.Resolved)` instead of `detection.AutoResolved` so the model stays consistent if the way auto-resolved items are tracked changes in the future.
- In `runReviewImport`, the bubbletea program is hard-wired to the real stdio while the rest of setup uses `streams`; if you ever need to support alternative IO (e.g., testing harnesses or wrapped streams) it may be cleaner to pass a custom `tea.ProgramOptions` wired to `streams` instead of `os.Stdin`/`os.Stdout`.
## Individual Comments
### Comment 1
<location path="cmd/dotagents/review.go" line_range="81" />
<code_context>
+
+func (m reviewModel) Init() tea.Cmd { return nil }
+
+func (m reviewModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) {
+ keyMsg, ok := msg.(tea.KeyMsg)
+ if !ok {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Avoid hard-coding the number of review actions when cycling with the space key.
Using `% 3` tightly couples the cycling logic to the current count of `reviewAction` values and will break if more actions are added. Derive the modulo value from the enum (e.g. `numActions := int(actionSkip) + 1` or a `const numReviewActions`) so the logic stays aligned with the definition.
Suggested implementation:
```golang
m.action = reviewAction((int(m.action) + 1) % numReviewActions)
```
Define the modulo value based on the reviewAction enum so it stays aligned when new actions are added. For example, near the reviewAction enum definition in cmd/dotagents/review.go, add:
const numReviewActions = int(actionSkip) + 1
(or equivalently derive it from the highest enum value), and ensure the name matches the usage in the updated line above.
</issue_to_address>
### Comment 2
<location path="cmd/dotagents/detect_test.go" line_range="183" />
<code_context>
if result.AutoResolved != 1 {
t.Fatalf("expected 1 auto-resolved (shared-skill), got %d", result.AutoResolved)
}
+ if len(result.Resolved) != 1 || result.Resolved[0].Name != "shared-skill" {
+ t.Fatalf("expected shared-skill in Resolved, got %+v", result.Resolved)
+ }
</code_context>
<issue_to_address>
**suggestion (testing):** Consider also asserting that AutoResolved matches len(Resolved) to lock in the intended invariant.
DetectionResult’s comment states AutoResolved is retained for JSON consumers and mirrors Resolved. This test checks AutoResolved and that Resolved contains the expected item, but not that AutoResolved == len(Resolved). Adding that assertion here would enforce the invariant and help catch future desyncs between the two fields.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a701b59267
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| continue | ||
| } | ||
| server := mcp.Server | ||
| server.Agents = []string{mcp.Origin} |
There was a problem hiding this comment.
When an MCP server is found in multiple harnesses, the decision retains only the selected/first Source, and this line scopes the canonical server to that one origin. Since desiredMCPServersForAgent honors server.Agents, subsequent syncs manage only that harness and leave the other detected copies unmanaged—even for items the UI reports as “shared automatically.” Preserve all detected source harnesses as targets while using the selected source only for the server payload.
Useful? React with 👍 / 👎.
Comment on lines
+152
to
+158
| for i, row := range m.rows { | ||
| line := fmt.Sprintf(" %-7s %-*s %s %s", row.item.Surface, nameWidth, row.item.Name, m.presence(row), m.actionCell(row)) | ||
| if i == m.cursor { | ||
| line = reviewCursorStyle.Render(line) | ||
| } | ||
| b.WriteString(line + "\n") | ||
| } |
There was a problem hiding this comment.
When the number of detected items exceeds the terminal height, View renders every row and the model neither handles tea.WindowSizeMsg nor maintains a viewport. The cursor can therefore move to rows that are outside the visible terminal area, preventing users with larger inventories from seeing which item or source they are changing before applying the import.
Useful? React with 👍 / 👎.
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.
Summary
(differ)marker, tri-state action↑↓/jknavigate,spacecycles share/keep/skip,←→picks the canonical source for differing items,a/sbulk share/skip,enterapplies,qaborts (abort skips import, setup continues)DetectionResultgains aResolvedlist so the apply phase knows which items were auto-resolvedcopyDirMissing, roleConvert,upsertCanonicalMCPServer); keep/skip leave native content untouched--jsonbanner fix also open as move setup banner after --json early return #128 (identical change, merges cleanly either way)Phase 2 of #126.
Test plan
go test ./...passes (8 new model/apply tests + detection tests updated)dotagents setupin a terminal with native skills present, verify table, actions, abortSummary by Sourcery
Introduce a unified terminal-based review screen for native content import during setup, with auto-resolution of identical items and mapping of review decisions back into the existing import mechanisms.
New Features:
Enhancements:
Build:
Tests: