feat(opencode): show Judgment Day rows in profiles#920
Conversation
|
Thanks for approving #907. I don't have permission to apply labels from this account, so this PR needs the |
Alan-TheGentleman
left a comment
Alan-TheGentleman
left a comment
There was a problem hiding this comment.
Hey @ninjoan, really nice work here. The design is clean: having ModelPickerRowsForProfile() delegate to ModelPickerRows() means profiles reuse the global row contract and the already-working apply/nav logic, so this stays low risk. Test coverage is solid, keeping JD out of "Set all SDD phases" is the right call (and tested), and the empty-providers option count fix (1 to 2) is a nice catch that aligns the count with the two options the screen already renders.
One change before merge: ClearModelPickerAssignment is exported and has two tests, but it has zero production callers and no key binding or help text exposes a clear action. As it stands it is dead code, and the tests give false confidence that clearing works in the UI when it is not reachable.
Two ways to resolve it:
- Wire it into the phase-list key handler (e.g. a
backspace/dbinding) and add it to the help text, so the feature is actually usable, or - Drop the function and its two tests until the PR that wires it up.
Verified locally on top of current main: build, tests, gofmt, and vet are all clean, and it merges cleanly. It complements the profile-scoped JD backend from 7620986 rather than conflicting with it. Once the dead-code point is sorted this is good to merge and will close #907.
📝 WalkthroughWalkthroughThe PR extends the Profile Create/Edit TUI flow to support Judgment Day (JD) agent model assignments. ChangesProfile Create/Edit: JD Agent Assignment Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Suggested labels
Suggested reviewers
🚥 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 |
|
Thanks for the clear review. I went with option 1 and wired the clear action into the production profile model-assignment flow. What changed:
Validation:
The effective diff is 355 changed lines, still under the 400-line budget. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/tui/screens/model_picker.go (1)
680-688:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winBack-option copy is incorrect in profile flow when providers are unavailable.
At Line 687, the fallback option label says
← Back to SDD mode, but in profile create step-1 the back path returns to profile flow (step 0 or profile list), not SDD mode. This is misleading UX.Suggested fix
- b.WriteString(renderOptions([]string{"Continue with defaults", "← Back to SDD mode"}, cursor)) + backLabel := "← Back to SDD mode" + if state.ForProfile { + backLabel = "← Back" + } + b.WriteString(renderOptions([]string{"Continue with defaults", backLabel}, cursor))🤖 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/screens/model_picker.go` around lines 680 - 688, The back option label in the renderOptions call is hardcoded as "← Back to SDD mode", but when this screen appears during profile creation (step 1), the back action returns to the profile flow, not to SDD mode. Update the second string in the renderOptions array to accurately reflect that the back option returns to the profile flow or profile list instead of SDD mode, so users understand where the back button will take them.
🤖 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.
Outside diff comments:
In `@internal/tui/screens/model_picker.go`:
- Around line 680-688: The back option label in the renderOptions call is
hardcoded as "← Back to SDD mode", but when this screen appears during profile
creation (step 1), the back action returns to the profile flow, not to SDD mode.
Update the second string in the renderOptions array to accurately reflect that
the back option returns to the profile flow or profile list instead of SDD mode,
so users understand where the back button will take them.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ad7a6e66-b04f-4423-8815-eb630753a38d
📒 Files selected for processing (6)
internal/tui/model.gointernal/tui/model_test.gointernal/tui/screens/model_picker.gointernal/tui/screens/model_picker_test.gointernal/tui/screens/profile_create.gointernal/tui/screens/profile_create_test.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/tui/screens/model_picker.go (1)
669-672: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueRedundant conditional branches — title is identical in both.
Both branches of the
if state.ForProfileconditional assign the same string totitle. This appears to be leftover from a refactor where the profile mode title was updated to match the non-profile title.♻️ Suggested simplification
- title := "Assign Models to SDD Phases & JD Agents" - if state.ForProfile { - title = "Assign Models to SDD Phases & JD Agents" - } + title := "Assign Models to SDD Phases & JD Agents"🤖 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/screens/model_picker.go` around lines 669 - 672, The title assignment in model_picker.go contains a redundant conditional branch where both the initial assignment and the if state.ForProfile block assign the identical string value "Assign Models to SDD Phases & JD Agents" to the title variable. Remove the entire if state.ForProfile conditional block and keep only the initial title assignment statement, since both branches produce the same result making the condition unnecessary.
🤖 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.
Outside diff comments:
In `@internal/tui/screens/model_picker.go`:
- Around line 669-672: The title assignment in model_picker.go contains a
redundant conditional branch where both the initial assignment and the if
state.ForProfile block assign the identical string value "Assign Models to SDD
Phases & JD Agents" to the title variable. Remove the entire if state.ForProfile
conditional block and keep only the initial title assignment statement, since
both branches produce the same result making the condition unnecessary.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6d57a15e-7082-4575-9b4d-4eefe171c2bf
📒 Files selected for processing (2)
internal/tui/model.gointernal/tui/screens/model_picker.go
2 participants
🔗 Linked Issue
Closes #907
🏷️ PR Type
What kind of change does this PR introduce?
type:bug— Bug fix (non-breaking change that fixes an issue)type:feature— New feature (non-breaking change that adds functionality)type:docs— Documentation onlytype:refactor— Code refactoring (no functional changes)type:chore— Build, CI, or tooling changestype:breaking-change— Breaking change (fix or feature that changes existing behavior)📝 Summary
📂 Changes
internal/tui/screens/model_picker.gointernal/tui/screens/profile_create.gointernal/tui/screens/*_test.go🧪 Test Plan
Focused tests
go test ./internal/tui/...)go test ./internal/components/sdd ./internal/cli ./internal/tui/...)cd e2e && ./docker-test.sh) — not run; TUI-only slice🤖 Automated Checks
The following checks run automatically on this PR:
additions + deletions) or usesize:exceptionCloses/Fixes/Resolves #Nstatus:approvedtype:*Labeltype:*label must be appliedgo test ./...must passcd e2e && ./docker-test.shmust pass✅ Contributor Checklist
status:approvedsize:exceptionwith rationale documentedtype:*label to this PR — maintainer label may be required because contributor token cannot label repo itemsgo test ./internal/tui/...)cd e2e && ./docker-test.sh) — not run; TUI-only sliceCo-Authored-Bytrailers💬 Notes for Reviewers
This is the first TUI slice after upstream added profile-scoped Judgment Day runtime/CLI support. It makes the JD slots visible in the OpenCode SDD Profile model picker. A follow-up slice will cover TUI state persistence/clear/default behavior, then docs.
Please add the
type:featurelabel if GitHub does not let the contributor apply it.Summary by CodeRabbit
New Features
Bug Fixes
Documentation