Port over basecamp cli improvements - #52
Conversation
- Enhanced User-Agent: include repo URL and SDK version for API observability - Added debug.ReadBuildInfo() fallback so go-install builds report real version - Linting: add contextcheck, goimports formatter, additional revive rules - Release script: colored output, --dry-run flag, jq check, existing-tag handling - Added .pre-commit-config.yaml with golangci-lint, gitleaks, and standard hooks Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace Cobra's default flat help with categorized, styled output: - Root help groups commands into EMAIL, CALENDAR & TASKS, AUTH & CONFIG - Subcommand help shows styled sections with curated inherited flags - Parent-scoped flags promoted into FLAGS, not buried in globals Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Ports CLI/TUI improvements inspired by Basecamp’s hey-cli, including a redesigned TUI navigation model, richer help output, and improved release/version metadata.
Changes:
- Replaced the Bubbletea “view-state + bubbles/list” TUI with a 2-row navigation header, help bar, and custom content lists/viewport threads.
- Added custom Cobra help rendering with curated root categories and consistent command help layout.
- Improved release tooling and version/user-agent metadata; upgraded
hey-sdkdependency and linting/pre-commit config.
Reviewed changes
Copilot reviewed 28 out of 29 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/release.sh | Adds validation, dry-run mode, tag handling, and nicer output for release tagging/push flow. |
| internal/version/version.go | Reads module build info for dev builds; expands user-agent; adds IsDev(). |
| internal/tui/tui.go | Major TUI architecture rewrite: sections/focus rows, viewport thread rendering, async loading/spinner, posting actions. |
| internal/tui/tui_test.go | Updates tests to match new TUI model (focus cycling, double-ctrl+c quit, ordering, lists, journal dates). |
| internal/tui/styles.go | Adds loading/error views + animation helpers; updates styling to ANSI-adaptive colors. |
| internal/tui/nav.go | New navigation model and header rendering (sections/subnav, box ordering/shortcuts). |
| internal/tui/help.go | New bottom help bar with dynamic key bindings and wrapping. |
| internal/tui/content.go | New custom mail/calendar list renderers (cursor, scrolling, formatting). |
| internal/tui/topic.go | Removes prior topic sub-model (replaced by shared viewport/thread rendering in tui.go). |
| internal/tui/detail.go | Removes prior detail sub-model (replaced by shared viewport/thread rendering in tui.go). |
| internal/tui/boxes.go | Removes legacy bubbles/list mailboxes model (superseded by new nav + content list). |
| internal/tui/box.go | Removes legacy bubbles/list box postings model (superseded by contentList). |
| internal/tui/calendars.go | Removes legacy bubbles/list calendars model (superseded by new nav + recording list). |
| internal/tui/calendar.go | Removes legacy bubbles/list calendar recordings model (superseded by recordingList). |
| internal/tui/journal.go | Removes legacy bubbles/list journal model (superseded by journal date subnav + viewport). |
| internal/tui/journal_test.go | Removes tests for legacy journal list behavior (no longer applicable). |
| internal/tui/calendar_test.go | Removes tests for legacy calendar list ordering behavior (no longer applicable). |
| internal/tui/debug_test.go | Removes debug-oriented tests (no longer applicable after input refactor). |
| internal/models/box.go | Expands Posting model to include fields used by new list UI (name, counts, extensions). |
| internal/cmd/root.go | Switches to the new custom help renderer. |
| internal/cmd/help.go | Adds custom help renderer (root curated sections + styled per-command help). |
| internal/cmd/writeok_test.go | Updates expectations for the new help output format. |
| internal/cmd/sdk.go | Appends SDK default UA to CLI UA; minor import ordering. |
| internal/client/client.go | Appends SDK default UA to CLI UA for legacy client requests too. |
| internal/cmd/formatting.go | Import grouping tweak. |
| go.mod | Bumps github.com/basecamp/hey-sdk/go to v0.2.0; removes unused indirect deps. |
| go.sum | Updates sums for hey-sdk bump and dependency pruning. |
| .pre-commit-config.yaml | Adds pre-commit hooks (lint, leaks, tidy, short tests). |
| .golangci.yml | Enables gofmt/goimports formatting and additional linters/rules. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if git rev-parse "$VERSION" >/dev/null 2>&1; then | ||
| EXISTING_SHA=$(git rev-parse "${VERSION}^{commit}") | ||
| if [[ "$EXISTING_SHA" == "$LOCAL" ]]; then | ||
| info "Tag $VERSION already exists at HEAD" | ||
| else | ||
| die "Tag $VERSION already exists at ${EXISTING_SHA:0:7} (not HEAD). Delete it first or choose a different version." | ||
| fi |
There was a problem hiding this comment.
git rev-parse "$VERSION" can succeed for non-tag refs (e.g., a branch named v1.2.3), which could make the script think a tag exists when it doesn’t. Prefer checking specifically for a tag ref (e.g., verify refs/tags/$VERSION) before treating it as an existing tag.
| # Verify required tools | ||
| if ! command -v jq >/dev/null 2>&1; then | ||
| die "jq is required but not found. Install with your package manager." | ||
| fi | ||
|
|
There was a problem hiding this comment.
This script doesn’t appear to use jq anywhere in the updated content, so hard-failing when jq is missing adds an unnecessary dependency for releases. Either remove this check or introduce the jq usage that requires it (and keep the check).
| # Verify required tools | |
| if ! command -v jq >/dev/null 2>&1; then | |
| die "jq is required but not found. Install with your package manager." | |
| fi |
| dateWidth := len(date) | ||
| prefixWidth := 4 // "│ ● " or " " | ||
| subjectWidth := max(c.width-prefixWidth-dateWidth-2, 10) // 2 for gap | ||
| if len(subject) > subjectWidth { | ||
| subject = subject[:subjectWidth-3] + "..." | ||
| } | ||
| gap := max(c.width-prefixWidth-len(subject)-dateWidth, 1) |
There was a problem hiding this comment.
Truncating subject via byte slicing can split multi-byte UTF-8 runes, producing invalid text and broken rendering in terminals. Use a rune-aware truncation approach (e.g., convert to []rune or truncate by display width with lipgloss.Width/runewidth) and then append ....
| dateWidth := len(date) | |
| prefixWidth := 4 // "│ ● " or " " | |
| subjectWidth := max(c.width-prefixWidth-dateWidth-2, 10) // 2 for gap | |
| if len(subject) > subjectWidth { | |
| subject = subject[:subjectWidth-3] + "..." | |
| } | |
| gap := max(c.width-prefixWidth-len(subject)-dateWidth, 1) | |
| dateWidth := lipgloss.Width(date) | |
| prefixWidth := 4 // "│ ● " or " " | |
| subjectWidth := max(c.width-prefixWidth-dateWidth-2, 10) // 2 for gap | |
| // Truncate subject by display width, avoiding splitting UTF-8 runes. | |
| if lipgloss.Width(subject) > subjectWidth { | |
| var b strings.Builder | |
| currentWidth := 0 | |
| ellipsisWidth := lipgloss.Width("...") | |
| for _, r := range subject { | |
| rw := lipgloss.Width(string(r)) | |
| if currentWidth+rw+ellipsisWidth > subjectWidth { | |
| break | |
| } | |
| b.WriteRune(r) | |
| currentWidth += rw | |
| } | |
| subject = b.String() + "..." | |
| } | |
| gap := max(c.width-prefixWidth-lipgloss.Width(subject)-dateWidth, 1) |
| if len(title) > subjectWidth { | ||
| title = title[:subjectWidth-3] + "..." | ||
| } |
There was a problem hiding this comment.
Same UTF-8 issue as the posting subject truncation: byte slicing can split runes and corrupt the output. Switch to rune-aware/display-width-aware truncation to keep rendering safe for non-ASCII titles.
| func (m model) handleKey(msg tea.KeyPressMsg) (tea.Model, tea.Cmd) { | ||
| key := msg.String() | ||
|
|
||
| // Double Ctrl+C to quit | ||
| if key == "ctrl+c" { | ||
| if m.ctrlCOnce { | ||
| m.cancel() | ||
| return m, tea.Quit | ||
| } | ||
| m.ctrlCOnce = true | ||
| m.updateHelpBindings() | ||
| return m, tea.Tick(2*time.Second, func(time.Time) tea.Msg { return ctrlCResetMsg{} }) | ||
| } |
There was a problem hiding this comment.
When m.inThread is true, most key presses never reach m.topicViewport.Update, so viewport-native controls (e.g., pgup/pgdn, home/end, etc.) won’t work, and keys like tab can still change focus rows even though the help text implies a “thread-only” control set. Consider short-circuiting in handleKey: if m.inThread and the key isn’t one of the global controls (ctrl+c, esc/q), pass the key msg directly to m.topicViewport.Update and return that cmd.
| } | ||
| return m, nil | ||
| } | ||
|
|
There was a problem hiding this comment.
When m.inThread is true, most key presses never reach m.topicViewport.Update, so viewport-native controls (e.g., pgup/pgdn, home/end, etc.) won’t work, and keys like tab can still change focus rows even though the help text implies a “thread-only” control set. Consider short-circuiting in handleKey: if m.inThread and the key isn’t one of the global controls (ctrl+c, esc/q), pass the key msg directly to m.topicViewport.Update and return that cmd.
| // In thread view, delegate all other keys to the topic viewport so that | |
| // viewport-native controls (pgup/pgdn, home/end, etc.) work as expected | |
| // and outer-level focus/section shortcuts are disabled. | |
| if m.inThread { | |
| var cmd tea.Cmd | |
| m.topicViewport, cmd = m.topicViewport.Update(msg) | |
| return m, cmd | |
| } |
| func generateJournalDates(n int) []string { | ||
| dates := make([]string, n) | ||
| today := time.Now() | ||
| for i := range n { | ||
| d := today.AddDate(0, 0, -(n - 1 - i)) | ||
| dates[i] = d.Format("2006-01-02") | ||
| } | ||
| return dates | ||
| } |
There was a problem hiding this comment.
for i := range n relies on Go 1.22+ “range over integers”. If this repo’s go directive targets < 1.22, this won’t compile. If older Go versions must be supported, replace with a classic indexed loop (for i := 0; i < n; i++ { ... }).
| today := time.Now().Format("2006-01-02") | ||
| if dates[6] != today { | ||
| t.Errorf("last date = %q, want today %q", dates[6], today) |
There was a problem hiding this comment.
This test can flake around midnight because generateJournalDates and time.Now() are called at different times. A simple stabilization is to capture now := time.Now() once in the test and compare against that, or (more robustly) refactor generateJournalDates to accept a “base time” for deterministic testing.
| today := time.Now().Format("2006-01-02") | |
| if dates[6] != today { | |
| t.Errorf("last date = %q, want today %q", dates[6], today) | |
| now := time.Now() | |
| today := now.Format("2006-01-02") | |
| yesterday := now.AddDate(0, 0, -1).Format("2006-01-02") | |
| if dates[6] != today && dates[6] != yesterday { | |
| t.Errorf("last date = %q, want %q or %q", dates[6], today, yesterday) |
| func customHelpFunc(defaultHelp func(*cobra.Command, []string)) func(*cobra.Command, []string) { | ||
| return func(cmd *cobra.Command, args []string) { | ||
| if agentFlag { | ||
| printAgentHelp(cmd) | ||
| return | ||
| } | ||
| if cmd == cmd.Root() { | ||
| renderRootHelp(cmd.OutOrStdout(), cmd) | ||
| return | ||
| } | ||
| renderCommandHelp(cmd) | ||
| } | ||
| } |
There was a problem hiding this comment.
The defaultHelp parameter is never used, which can confuse readers and suggests an intended fallback path that doesn’t exist. Either remove the parameter (and update the call site), or call defaultHelp(cmd, args) where you want Cobra’s standard help output to be used.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 29 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/tui/styles.go">
<violation number="1" location="internal/tui/styles.go:127">
P3: Use a display-width calculation (e.g., lipgloss.Width or runewidth.StringWidth) instead of len() when wrapping/padding error text; len() counts bytes and will misalign the error box for non-ASCII messages.</violation>
</file>
<file name="scripts/release.sh">
<violation number="1" location="scripts/release.sh:73">
P2: The new jq requirement is unused in this script and in the release-check target, so it can block releases on machines that don’t have jq even though nothing here needs it. Remove this check unless a later step actually uses jq.</violation>
</file>
<file name="internal/tui/content.go">
<violation number="1" location="internal/tui/content.go:139">
P2: Use rune-/display-width truncation for subject so Unicode subjects don’t get split or misaligned in the TUI.</violation>
<violation number="2" location="internal/tui/content.go:298">
P2: Use rune-/display-width truncation for recording titles so Unicode titles render safely and align correctly.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| echo " Branch: $CURRENT_BRANCH" | ||
| echo " Commit: $LOCAL" | ||
| # Verify required tools | ||
| if ! command -v jq >/dev/null 2>&1; then |
There was a problem hiding this comment.
P2: The new jq requirement is unused in this script and in the release-check target, so it can block releases on machines that don’t have jq even though nothing here needs it. Remove this check unless a later step actually uses jq.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/release.sh, line 73:
<comment>The new jq requirement is unused in this script and in the release-check target, so it can block releases on machines that don’t have jq even though nothing here needs it. Remove this check unless a later step actually uses jq.</comment>
<file context>
@@ -23,58 +48,72 @@ DEFAULT_BRANCH="${DEFAULT_BRANCH:-main}"
-echo " Branch: $CURRENT_BRANCH"
-echo " Commit: $LOCAL"
+# Verify required tools
+if ! command -v jq >/dev/null 2>&1; then
+ die "jq is required but not found. Install with your package manager."
+fi
</file context>
| dateWidth := len(date) | ||
| prefixWidth := 4 // "│ ● " or " " | ||
| subjectWidth := max(c.width-prefixWidth-dateWidth-2, 10) // 2 for gap | ||
| if len(subject) > subjectWidth { |
There was a problem hiding this comment.
P2: Use rune-/display-width truncation for subject so Unicode subjects don’t get split or misaligned in the TUI.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/tui/content.go, line 139:
<comment>Use rune-/display-width truncation for subject so Unicode subjects don’t get split or misaligned in the TUI.</comment>
<file context>
@@ -0,0 +1,333 @@
+ dateWidth := len(date)
+ prefixWidth := 4 // "│ ● " or " "
+ subjectWidth := max(c.width-prefixWidth-dateWidth-2, 10) // 2 for gap
+ if len(subject) > subjectWidth {
+ subject = subject[:subjectWidth-3] + "..."
+ }
</file context>
| } | ||
|
|
||
| subjectWidth := max(c.width-4-len(date)-2, 10) | ||
| if len(title) > subjectWidth { |
There was a problem hiding this comment.
P2: Use rune-/display-width truncation for recording titles so Unicode titles render safely and align correctly.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/tui/content.go, line 298:
<comment>Use rune-/display-width truncation for recording titles so Unicode titles render safely and align correctly.</comment>
<file context>
@@ -0,0 +1,333 @@
+ }
+
+ subjectWidth := max(c.width-4-len(date)-2, 10)
+ if len(title) > subjectWidth {
+ title = title[:subjectWidth-3] + "..."
+ }
</file context>
| lines := wrapText(errMsg, maxInner) | ||
| innerWidth := 0 | ||
| for _, l := range lines { | ||
| if len(l) > innerWidth { |
There was a problem hiding this comment.
P3: Use a display-width calculation (e.g., lipgloss.Width or runewidth.StringWidth) instead of len() when wrapping/padding error text; len() counts bytes and will misalign the error box for non-ASCII messages.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/tui/styles.go, line 127:
<comment>Use a display-width calculation (e.g., lipgloss.Width or runewidth.StringWidth) instead of len() when wrapping/padding error text; len() counts bytes and will misalign the error box for non-ASCII messages.</comment>
<file context>
@@ -1,23 +1,168 @@
+ lines := wrapText(errMsg, maxInner)
+ innerWidth := 0
+ for _, l := range lines {
+ if len(l) > innerWidth {
+ innerWidth = len(l)
+ }
</file context>
No description provided.