Conversation
gtsiolis
requested review from
anisaoshafi,
carole-lavillonniere and
silv-io
as code owners
gtsiolis
force-pushed
the
george/improve-prompt-for-profile
branch
2 times, most recently
from
c0e7cc2 to
706b249
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughRemoved dynamic files-to-modify logic and its test; replaced AWS config prompt and success text with fixed strings; refactored prompt rendering to support multi-line prompts and move option-label formatting into a helper; added unit tests for InputPrompt and updated integration tests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/ui/components/input_prompt.go (1)
42-75: Add a focusedInputPrompt.View()test matrix.This branch now owns prompt formatting locally, but the updated coverage only exercises one integration path and a substring match. A small component test table for no options / one option / multiple options / multi-line prompts would catch rendering drift without coupling TUI output back to the plain formatter.
Based on learnings: In reviews of code under
internal/ui/components, allow TUI components to implement their own event rendering logic distinct from the plain-output rendering ininternal/output/format.go. Ensure tests verify the separate rendering paths and avoid unintended coupling to the plain-output formatting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ui/components/input_prompt.go` around lines 42 - 75, Add a focused table-driven unit test for InputPrompt.View() that exercises the component's own rendering logic across the key cases: no options, one option, multiple options, and multi-line prompts; for each case construct an InputPrompt with appropriate prompt text and p.options (including labels), call View(), and assert the exact rendered string includes the secondary "?" prefix, message styling output for the first line, correctly formatted single "(label)" or multiple "[a/b]" option label segments, and the secondary-styled trailing lines for multi-line prompts—ensure the tests call InputPrompt.View() directly (not the plain formatter in internal/output/format.go) so they validate the TUI component rendering path independently.internal/awsconfig/awsconfig.go (1)
224-246: RemoveprofileStatus.filesToModify()now that the copy is fixed.Hardcoding the prompt and success text seems to have eliminated the last reason for
profileStatus.filesToModify(). Dropping that helper would keepprofileStatusfocused on detection only and avoid stale message-formatting logic hanging around.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/awsconfig/awsconfig.go` around lines 224 - 246, Remove the now-unused profileStatus.filesToModify() helper and any calls to it so profileStatus is only responsible for detection; keep the current hardcoded prompt and success messages in the interactive flow, and ensure the branch that writes files still uses status.configNeeded and status.credsNeeded to call writeConfigProfile and writeCredsProfile (and emits warnings on error) and then emits the existing success string; delete the filesToModify() method and any stale formatting logic tied to it from the profileStatus type and related code paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/awsconfig/awsconfig.go`:
- Around line 224-246: Remove the now-unused profileStatus.filesToModify()
helper and any calls to it so profileStatus is only responsible for detection;
keep the current hardcoded prompt and success messages in the interactive flow,
and ensure the branch that writes files still uses status.configNeeded and
status.credsNeeded to call writeConfigProfile and writeCredsProfile (and emits
warnings on error) and then emits the existing success string; delete the
filesToModify() method and any stale formatting logic tied to it from the
profileStatus type and related code paths.
In `@internal/ui/components/input_prompt.go`:
- Around line 42-75: Add a focused table-driven unit test for InputPrompt.View()
that exercises the component's own rendering logic across the key cases: no
options, one option, multiple options, and multi-line prompts; for each case
construct an InputPrompt with appropriate prompt text and p.options (including
labels), call View(), and assert the exact rendered string includes the
secondary "?" prefix, message styling output for the first line, correctly
formatted single "(label)" or multiple "[a/b]" option label segments, and the
secondary-styled trailing lines for multi-line prompts—ensure the tests call
InputPrompt.View() directly (not the plain formatter in
internal/output/format.go) so they validate the TUI component rendering path
independently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 1d22df7f-475d-4879-a7d9-952ccd4f4bc8
📒 Files selected for processing (3)
internal/awsconfig/awsconfig.gointernal/ui/components/input_prompt.gotest/integration/awsconfig_test.go
| // "?" prefix in secondary color | ||
| sb.WriteString(styles.Secondary.Render("? ")) | ||
|
|
||
| // Style trailing "?" with secondary color |
There was a problem hiding this comment.
issue: we never seem to be styling the "trailing ? with secondary color" like the comment says, so not sure what is the intent of this code
There was a problem hiding this comment.
Oops, seems like a leftover as I was trying to have only the the prefix "?" use secondary color.
Nice catch! ⚾ Changed in 1790406.
| sb.WriteString(styles.SecondaryMessage.Render(strings.Join(lines[1:], "\n"))) | ||
| } | ||
|
|
||
| return sb.String() |
There was a problem hiding this comment.
suggestion: it looks like the logic to format the labels is duplicated in plain_format.go:62-89. Can we introduce a new helper FormatPromptLabels that's used in both places?
gtsiolis
force-pushed
the
george/improve-prompt-for-profile
branch
from
c763b2e to
fa61ef1
Compare
2 participants
Some improvements on the input prompt for AWS profile for #92.
Open to rebasing and merging afterwards.