Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
|
||
| // View renders `[ ] Label` / `[x] Label`. | ||
| func (c *CheckboxField) View() string { | ||
| styles := theme.DefaultStyles() |
There was a problem hiding this comment.
Form fields allocate new styles on every render
Low Severity
Every form field widget (CheckboxField, TextField, SelectField, and the Form container) calls theme.DefaultStyles() inside its View() method, which allocates a new *Styles struct on every render frame. Since View() is called on every keystroke and there can be many fields, this creates unnecessary repeated allocations. More importantly, this hard-codes default styles and ignores the *theme.Styles instance the parent page already carries, making future style customization impossible for form internals.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 272c6d1. Configure here.
Adds the router/Page/theme/form/registry skeletons that phase 2+ fill in, alongside the existing TUI which still runs unchanged. Also deletes the Soft Serve-era dead components (header, footer, statusbar, viewport, confirm, formfield, textinput), the unused glamour StyleConfig, and the unused Styles fields (Repo, Log, Tree, StatusBar*, Ref, Spinner, Error*, TopLevel*, Branch, ServerName, etc.). Made-with: Cursor Co-authored-by: Miccah Castorina <[email protected]>
Fills in pkg/tui/components/form with working TextField, CheckboxField, SelectField and SecretField widgets; a Form container that drives focus, validation and SubmitMsg emission; the pure BuildArgs() / Form.Args() function that emits the kingpin arg vector per EmitMode; and the Required / Integer / OneOf / XOrGroup validators the sources need. Tests cover every EmitMode, every validator, an XOrGroup fixture modeled on GitHub's org/repo mutex, and an end-to-end Form.Args() round trip including a path with a space. Made-with: Cursor Co-authored-by: Miccah Castorina <[email protected]>
Every source package now exports a Definition() that wires form.FieldSpecs, validators, and (where needed) a BuildArgs override into the new registry. Each package registers itself in init(); pkg/tui/tui.go blank-imports them to force registration. sources.CmdModel.Cmd() now returns []string, so values carrying spaces (filesystem paths, docker images, etc.) survive the hand-off to kingpin without shell-quoting. source_configure threads that []string through run_component + tui.go's SetArgsMsg consumer. The three TruffleHog-wide booleans (--json, --no-verification, --only-verified → --results=verified) are now checkboxes instead of free-text "true"/"false" fields. Form field navigation is rebound to up/down so it doesn't collide with the tabs component, which still owns tab / shift+tab. Verification: go build ./..., go vet ./pkg/tui/..., and new pkg/tui/sources/sources_test.go covering registry completeness plus arg-vector tables for every registered source (including space-in-positional round-trip, repeated --image=/--bucket= emission, and the elasticsearch / jenkins / postman BuildArgs overrides). Made-with: Cursor Co-authored-by: Miccah Castorina <[email protected]>
Every page now implements app.Page and communicates through navigation messages (PushMsg, PopMsg, ExitMsg, RunAnalyzerMsg) instead of shared global state. Directory layout moves to kebab-case: wizard/, source-picker/, source-config/, analyzer-picker/, analyzer-form/, link-card/. The view_oss and contact_enterprise pages collapse into one link-card page parametrised by title + URL. pkg/tui/tui.go becomes the thin runner: build app.Model, register page factories, decide the initial page from argv (preserves the no-args / analyze / analyze <known> contract), hand back Args or analyzer SecretInfo. source-config uses ResizeMsg to drive its form sizing and advances tabs on form.SubmitMsg (enter on last field) or the Section binding (tab / shift+tab). Run tab emits ExitMsg with the assembled argv. analyzer-form retires the legacy textinputs model for form.Form with the same per-analyzer field layouts; it emits RunAnalyzerMsg on submit so the router hands off to analyzer.Run without going through kingpin. Verification: go build ./..., go vet ./pkg/tui/..., go test ./pkg/tui/... (form + sources + new tui_test.go covering all initialPage entry paths). Made-with: Cursor Co-authored-by: Miccah Castorina <[email protected]>
Retires the legacy TUI infrastructure that phases 1-4 replaced:
pkg/tui/common (god-struct + utils), pkg/tui/keymap, pkg/tui/styles,
pkg/tui/components/{textinputs,selector,tabs}, and the sources shim
file holding CmdModel + GetSourceFields/GetSourceNotes. Source lookup
is now id-keyed end-to-end; source-picker pushes source-config with
the Definition.ID and source-config calls sources.Get directly.
Also cleans the theme package comments that referenced the now-deleted
predecessor packages.
Verification: go build ./..., go vet ./pkg/tui/..., go test ./pkg/tui/...
all green with the legacy tree removed.
Made-with: Cursor
Co-authored-by: Miccah Castorina <[email protected]>
Lightweight architecture overview and workflow diagrams (entry, source-scan, analyzer) plus brief recipes for adding a new source and a new page. Keeps the single source of truth for TUI shape next to the code instead of in an external doc. Made-with: Cursor Co-authored-by: Miccah Castorina <[email protected]>
| m.args = nil | ||
| return tea.Quit | ||
| } | ||
| return m.pop() |
There was a problem hiding this comment.
Router intercepts esc key, breaking list filter cancellation
High Severity
The router's handleGlobalKey unconditionally intercepts the esc key (bound to Back) and pops the page or quits — before the message ever reaches the active page. Both the source picker and analyzer picker use bubbles/list with filtering enabled, which relies on esc to cancel an active filter. When a user activates the filter and presses esc to dismiss it, the router pops the page instead, making list filtering effectively unusable.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 637df36. Configure here.
| constraints: constraints, | ||
| submitKeys: key.NewBinding(key.WithKeys("ctrl+s")), | ||
| nextKeys: key.NewBinding(key.WithKeys("down")), | ||
| prevKeys: key.NewBinding(key.WithKeys("up")), |
There was a problem hiding this comment.
Form down/up keys conflict with text input cursor
Medium Severity
The Form container binds down and up to nextKeys/prevKeys for focus cycling, but these keys are intercepted before the focused TextField ever sees them. This means users cannot use the up/down arrow keys for cursor history or multi-line navigation within a text input — pressing down always moves focus to the next field, even mid-edit. The old implementation used enter for field navigation, not arrows, alongside the focused text input.
Reviewed by Cursor Bugbot for commit 888020e. Configure here.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 4 total unresolved issues (including 3 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 26d64bd. Configure here.
| v := values[s.Key] | ||
| if s.Transform != nil { | ||
| v = s.Transform(strings.TrimSpace(v)) | ||
| } |
There was a problem hiding this comment.
Transform applied before TrimSpace causes double-trim inconsistency
Medium Severity
BuildArgs trims the value before passing it to Transform, then checks strings.TrimSpace(v) again on the transformed result to decide emission. But the emitted value v is the Transform output (not trimmed again), so a Transform that adds whitespace would emit those extra spaces. More critically, for EmitLongFlag the untrimmed transformed value is emitted as a separate token: --key followed by the value, which may have unexpected leading/trailing whitespace from the Transform.
Reviewed by Cursor Bugbot for commit 26d64bd. Configure here.
1 participant
Description:
Explain the purpose of the PR.
Checklist:
make test-community)?make lintthis requires golangci-lint)?Note
Medium Risk
Large refactor of the interactive TUI flow (navigation, form handling, and argv/analyzer handoff) could introduce regressions in user interaction and generated CLI arguments, though changes are mostly isolated to
pkg/tuiand are covered by new unit tests.Overview
Overhauls the TUI into a router + page stack (
pkg/tui/app) where pages communicate only via navigation messages (PushMsg/PopMsg/ExitMsg/RunAnalyzerMsg), centralizing global key handling, resizing, and the handoff back to kingpin/analyzer.Replaces the legacy ad-hoc components and textinput-driven command building with a declarative form system (
pkg/tui/components/form) and a source registry (pkg/tui/sources/registry.go), migrating OSS sources tosources.Definitionentries (with selectiveBuildArgsoverrides for complex emission) and adding tests to assert emitted arg vectors.Reimplements the main user flows as new pages (
wizard,source-picker,source-config,analyzer-picker,analyzer-form,link-card), removes the old page/component implementations, and documents the new architecture inpkg/tui/README.md.Reviewed by Cursor Bugbot for commit 26d64bd. Bugbot is set up for automated code reviews on this repo. Configure here.