feat: pkg/prompt #4802
feat: pkg/prompt #4802
Conversation
to be used in a nice seed util and probably our cli
chronark
requested review from
Flo4604,
MichaelUnkey,
imeyer,
mcstepp,
ogzhanolguncu and
perkinsjr
as code owners
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
📝 WalkthroughWalkthroughAdds a new interactive terminal prompt package (pkg/prompt) with String/Int/Float/Select/MultiSelect prompts, Date/Time/DateTime pickers, human-friendly number/date parsing, a Wizard wrapper, comprehensive tests and examples, and Bazel + Go module changes to depend on golang.org/x/term. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Command failed 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: 4
🤖 Fix all issues with AI agents
In `@pkg/prompt/datetime_parse.go`:
- Around line 150-213: The parser currently ignores seconds in "HH:MM:SS" and
allows invalid 12-hour inputs like "13pm" or "0am"; update the parsing in the
function handling time strings to explicitly handle three-part times (when
strings.Contains(s, ":") and split yields 3 parts) by parsing seconds,
validating seconds in 0–59, converting the hour through convert12To24, and
returning an error on any out-of-range component; additionally, when isAM or
isPM is set (detected by suffix handling for "am"/"pm"/"a"/"p"), enforce 12-hour
source validation so the raw hour before convert12To24 must be in 1–12 (reject 0
or >12) and apply the same stricter checks in the other similar parsing branch
referenced (the block around lines 218-226) so lone-hour parses with AM/PM also
reject invalid 12-hour values.
- Around line 50-76: The parsed month/day/year from the "/" and "." branches are
not validated before calling time.Date (which silently normalizes invalid
dates); after converting with strconv.Atoi in both blocks (the parts ->
month/day/year variables), validate month is 1..12 and day is >=1 and <=
daysInMonth(year, month) (accounting for leap years) and only then call
time.Date; if validation fails, return a parsing error. Add or reuse a small
helper (e.g., daysInMonth(year, month)) and apply this check in both the
slash-parsing and dot-parsing branches so malformed inputs like 13/40/2024 are
rejected instead of normalized.
In `@pkg/prompt/parse.go`:
- Around line 3-7: The integer parsing paths can overflow because you convert or
flip sign before validating bounds: in the decimal-suffix branch
convert/checking `result` to int64 only after potential overflow, and in the
integer-suffix branch the sign-flip check using `base`/`multiplier` is
incomplete (negatives with positive multiplier can overflow undetected). Fix
both by performing bounds-first checks: in the decimal path validate that the
unsigned/unsigned-accumulated value is within int64 limits before casting to
int64 (do not cast `result` early), and in the integer path use
pre-multiplication overflow checks using `base` and `multiplier` (e.g., ensure
`abs(result) <= maxInt/abs(multiplier)` or equivalent) before multiplying or
flipping sign so no operation can trigger undefined overflow; update the
branches that reference `result`, `base`, and `multiplier` accordingly.
In `@pkg/prompt/prompt.go`:
- Around line 158-162: The code builds a slice of map keys from options which
yields non-deterministic ordering; to fix, sort the keys before iterating so
Select and MultiSelect render options in a stable order. In the prompt functions
that build keys (the snippet creating keys := make([]string, 0, len(options))
and appending map keys) add sort.Strings(keys) (import "sort") before using the
slice, and apply the same change to the other occurrence that constructs keys
(the block also referenced for lines 248-251); ensure any logic that maps
defaultKey or indices uses the sorted keys so default selection remains correct.
🧹 Nitpick comments (12)
MODULE.bazel (1)
89-89: Consider movingorg_golang_x_termto be with otherorg_golang_x_*entries.The
org_golang_x_termentry is placed after the linter dependencies section, but otherorg_golang_x_*packages (org_golang_x_net,org_golang_x_sync,org_golang_x_text,org_golang_x_tools) are grouped together at lines 78-81. For consistency and maintainability, consider moving this entry to that group.pkg/prompt/ansi.go (1)
64-69: Consider adding a guard for negative inputs toitoa.The comment correctly notes this is for "small positive integers," but negative input would cause infinite recursion (stack overflow). Since this is internal and only used for cursor counts, it's low risk, but a simple guard would make it defensive.
🛡️ Optional defensive guard
func itoa(n int) string { + if n < 0 { + return "0" // or handle error appropriately + } if n < 10 { return string(rune('0' + n)) } return itoa(n/10) + string(rune('0'+n%10)) }pkg/prompt/parse.go (1)
45-118: Preferfaultfor structured errors in parsing helpers.
These new helpers return many rawfmt.Errorf/strconverrors; the codebase guideline prefersfault.Wrap()/fault.Public()for consistent error semantics.As per coding guidelines, consider switching these returns to
fault-based errors.Also applies to: 139-172
pkg/prompt/datetime_parse.go (1)
21-215: Preferfaultfor structured errors in date/time parsing.
There are manyfmt.Errorfreturns; the repo guideline prefersfault.Wrap()/fault.Public()for consistent error handling.As per coding guidelines, consider switching these returns to
fault-based errors.pkg/prompt/parse_test.go (1)
205-213: Add build tags to skip 32-bit platform tests or document 64-bit-only requirement.The test values (int64 max/min) exceed 32-bit int bounds and would cause the overflow check to return an error on 32-bit systems, making these tests fail on 386 and ARM architectures. While the function correctly uses platform-dependent
maxInt/minIntbounds, the test itself is locked to 64-bit values.Consider either adding
//go:build amd64 || arm64 || (other 64-bit archs)to skip on 32-bit platforms, or document that this package requires 64-bit systems.pkg/prompt/datetime_parse_test.go (1)
10-18: Stabilize tests that depend ontime.Now().
ParseDate/ParseTime/ParseDateTimecalltime.Now()internally while the tests snapshottime.Now()separately; this can flake at minute/day boundaries. Consider anowFntest hook indatetime_parse.goor loosen assertions to accept a before/after window.✅ Example tweak for the “now” time test
- now := time.Now() - hour, minute, err := ParseTime("now") - require.NoError(t, err) - require.Equal(t, now.Hour(), hour) - require.Equal(t, now.Minute(), minute) + before := time.Now() + hour, minute, err := ParseTime("now") + after := time.Now() + require.NoError(t, err) + require.True(t, + (hour == before.Hour() && minute == before.Minute()) || + (hour == after.Hour() && minute == after.Minute()), + "parsed time should match before/after snapshot", + )Also applies to: 140-146, 277-315
pkg/prompt/wizard_test.go (1)
260-311: Consider using a helper or option pattern instead of accessing unexported fields.Direct manipulation of
wiz.prompt.inworks because tests are in the same package, but it couples tests to internal implementation details. Consider adding a test helper or using the existingWithReaderoption to configure new prompts for each step.♻️ Suggested approach
func TestWizardMultipleSteps(t *testing.T) { t.Run("completes full workflow", func(t *testing.T) { out := &bytes.Buffer{} + // Use a combined reader or reset via public API if available in1 := strings.NewReader("step1\n") p := New(WithReader(in1), WithWriter(out)) wiz := p.Wizard(3) r1, err := wiz.String("First") require.NoError(t, err) require.Equal(t, "step1", r1) require.Equal(t, 1, wiz.Current()) - in2 := strings.NewReader("step2\n") - wiz.prompt.in = in2 + // Alternative: Create a custom io.Reader that can be fed sequentially + // or accept this as test-only internal accesspkg/prompt/datetime.go (3)
246-305: Consider extracting shared input handling to reduce nesting complexity.The static analysis tool flagged this block with complexity 30 due to deep nesting. The input handling patterns for date navigation, time adjustment, and focus switching are repeated across
Date,Time, andDateTimemethods.Consider extracting the common patterns:
- A helper for date navigation (arrow keys, PgUp/PgDn, Home/End)
- A helper for time adjustment (hour/minute with focus)
- Common key handling for Enter/Ctrl+C
This would reduce duplication and make each picker easier to maintain.
337-386: Opportunity to reduce duplication in calendar rendering.The calendar grid rendering logic in
renderCalendar(lines 347-376) andrenderDateTimePicker(lines 437-470) is nearly identical, differing only in the color treatment based on focus state. Consider extracting a shared helper that accepts a focus parameter.♻️ Suggested helper extraction
// renderCalendarGrid generates the day grid portion of a calendar. // If dimmed is true, uses dim colors for non-selected days. func (p *Prompt) renderCalendarGrid(viewMonth, selected time.Time, focused bool) []string { // Shared logic for grid generation with color determined by focused flag }Also applies to: 414-470
104-107: Consider using a sentinel error for "interrupted" instead offmt.Errorf.The "interrupted" error is created with
fmt.Errorfin three places. Consider defining a package-level sentinel error for consistent error checking by callers.♻️ Suggested change
// At package level (e.g., in prompt.go or errors.go) var ErrInterrupted = errors.New("interrupted") // Then in each handler: return time.Time{}, ErrInterruptedAlso applies to: 187-190, 321-324
pkg/prompt/wizard.go (1)
48-73: Consider adding bounds validation to Skip and advance.Both
Skip()andadvance()incrementcurrentwithout checking if it exceedstotal. While this may be acceptable if callers are expected to track completion, it could lead to confusing progress indicators (e.g.,[●●●●●]with 5 dots for a 3-step wizard).♻️ Suggested bounds check
func (w *Wizard) Skip() { + if w.current < w.total { w.current++ + } } func (w *Wizard) advance() { + if w.current < w.total { w.current++ + } }pkg/prompt/prompt.go (1)
221-224: Consider a sentinel error for "interrupted".Similar to the suggestion for
datetime.go, the "interrupted" error is created withfmt.Errorf. A package-levelErrInterruptedwould allow callers to check for interrupts witherrors.Is().Also applies to: 331-334
* feat: pkg/prompt to be used in a nice seed util and probably our cli * [autofix.ci] apply automated fixes --------- Co-authored-by: Flo <53355483+Flo4604@users.noreply.github.com> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
3 participants
to be used in a nice seed util and probably our cli