feat(tui): add full-screen Bubble Tea TUI to hooksctl forward#10
Merged
Conversation
Proposal, design, spec, and tasks for the full-screen Bubble Tea status dashboard for `hooksctl forward`.
- Commit to Bubble Tea v2; drop stale "pin v1" caveat - Single-phase quit (drop two-phase draining state machine) - Visual-only pause/resume (no back-channel to SSE goroutine) - Remove open-browser keybind (w) — no concrete use case - Defer replay keybind (r) to v2 — requires undefined API client - Specify column-drop thresholds: suffix <80, size <73, latency <65 - Toast replaces keybind bar for 1.5s rather than appearing below it
Implements the hooksctl-tui OpenSpec change. When stdout is a TTY, `hooksctl forward` now boots into an ngrok-style dashboard instead of logging to stdout. The non-TTY path (CI/pipe) is unchanged. - New internal/tui package: Bubble Tea v2 model with ring-buffered delivery log (cap 500), live session header (state pill, uptime, account, route, token fingerprint), responsive layout (column drop <80/<73/<65 cols, identity collapse <24 rows), keybind bar with clipboard copy, visual pause/resume, and help overlay. - forward.go: TTY detection via charmbracelet/x/term; TUI path runs SSE consumer in a goroutine and sends DeliveryReceivedMsg / DeliveryCompletedMsg / SessionStateMsg to the Bubble Tea program. - Dependencies: charm.land/bubbletea/v2, charm.land/bubbles/v2, github.com/atotto/clipboard.
- Extract parseEventPayload and streamFromCursorWith to eliminate duplicated SSE parsing and JSON/base64 logic across TUI and non-TUI paths - Move stderr logging out of parseEventPayload so writes don't corrupt the alt-screen during TUI operation; non-TUI forwardOne logs instead - Add defer prog.RestoreTerminal() panic-safety net in runWithTUI - Auto-send QuitMsg when server closes stream cleanly, matching non-TUI exit behavior - Log reconnect errors to stderr before backoff delay - Fix esc key to only dismiss the help overlay, not open it (separate dismiss binding) - Add cancelled suffix for context-cancelled in-flight deliveries - Remove dead StatePaused constant and help.Model field - Remove dead stderr write from copyURLCmd (toast is the TUI feedback channel) - Flatten nested if in runWithTUI clean-close branch - Use key binding Help() values in renderKeybindBar instead of hardcoded strings - Add renderHelpOverlay sync comment - Add unit tests: parseEventPayload, tokenFingerprint, streamFromCursorWith errSkipEvent path - Add tests: copy URL key, esc open/close behavior, scroll position preserved on new delivery - Fix TestUpdate_ToastLifecycle to not block 1.5s on the one-shot timer
📝 WalkthroughWalkthroughThis PR introduces a full-screen Bubble Tea TUI for ChangesBubble Tea TUI for hooksctl forward
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 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 docstrings
🧪 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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/tui/types.go (1)
8-12: 💤 Low valueConsider documenting StateOffline in the spec.
The
SessionStateenum includesStateOffline(line 11), but the spec (specs/forward-tui/spec.md) only documents scenarios for online, reconnecting, and paused states. IfStateOfflineis used in practice, it should be documented in the spec for completeness.🤖 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/types.go` around lines 8 - 12, SessionState defines an extra value StateOffline that isn’t described in the spec; update the TUI spec to add documentation for StateOffline including its semantic meaning, valid transitions to/from StateOnline and StateReconnecting, when it should be emitted/observed, and the expected UI behavior (e.g., indicators, user actions or retry logic). Reference the enum name SessionState and the literal StateOffline in the spec text so readers can correlate the code and include any examples or sequence diagrams showing transitions involving StateOffline.
🤖 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.
Inline comments:
In `@internal/tui/view.go`:
- Around line 214-220: fixedHeaderRows overestimates identity height because it
always uses 4 rows for tall terminals while renderIdentity emits 3 rows when
Email is empty; change fixedHeaderRows to compute identityRows the same way
renderIdentity does (use 2 rows when termH < 24, otherwise 3 if identity.Email
is empty, 4 if not). Implement this by adding a parameter (e.g., hasEmail bool
or identity struct) to fixedHeaderRows and updating all callers to pass the
identity email presence, then adjust the return calculation (1 + identityRows +
1 + 1 + 1 + 1) accordingly.
In `@openspec/changes/hooksctl-tui/tasks.md`:
- Around line 28-31: The checklist incorrectly references a removed pause
keybinding; remove or update the stale items that mention the `pause` key from
the checklist entries that list `keyMap` bindings and the Update wiring (items
mentioning `p`, `pause`, and pause/resume behavior). Specifically, edit the
checklist so `keyMap` only documents the current bindings (e.g., `copyURL`,
`help`, `quit`), and remove any TODOs about implementing `ShortHelp()`,
`FullHelp()` or `Update()` wiring for a `pause`/`p` binding so the checklist
matches the actual `keyMap`, `ShortHelp`, `FullHelp`, and `Update` behavior.
---
Nitpick comments:
In `@internal/tui/types.go`:
- Around line 8-12: SessionState defines an extra value StateOffline that isn’t
described in the spec; update the TUI spec to add documentation for StateOffline
including its semantic meaning, valid transitions to/from StateOnline and
StateReconnecting, when it should be emitted/observed, and the expected UI
behavior (e.g., indicators, user actions or retry logic). Reference the enum
name SessionState and the literal StateOffline in the spec text so readers can
correlate the code and include any examples or sequence diagrams showing
transitions involving StateOffline.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5850b9dc-9391-402b-8183-e44b26af8be5
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (16)
.gitignorecmd/hooksctl/forward.gocmd/hooksctl/forward_unit_test.gogo.modinternal/tui/commands.gointernal/tui/keys.gointernal/tui/model.gointernal/tui/styles.gointernal/tui/tui_test.gointernal/tui/types.gointernal/tui/view.goopenspec/changes/hooksctl-tui/.openspec.yamlopenspec/changes/hooksctl-tui/design.mdopenspec/changes/hooksctl-tui/proposal.mdopenspec/changes/hooksctl-tui/specs/forward-tui/spec.mdopenspec/changes/hooksctl-tui/tasks.md
Comment on lines
+28
to
+31
| - [x] 5.1 Define `keyMap` struct with `key.Binding` fields: `copyURL`, `pause`, `help`, `quit` | ||
| - [x] 5.2 Implement `ShortHelp()` and `FullHelp()` on `keyMap` for the bubbles `help.Model` | ||
| - [x] 5.3 Wire key bindings in `Update()` — `c`, `p`, `?`, `q`, `ctrl+c` | ||
|
|
There was a problem hiding this comment.
Remove stale pause keybinding tasks from the checklist.
These checklist items still describe p pause/resume behavior, but the current TUI implementation and reviewer notes indicate pause keybinding is no longer part of the shipped interaction model. This can mislead future maintenance/testing.
Also applies to: 42-42
🤖 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 `@openspec/changes/hooksctl-tui/tasks.md` around lines 28 - 31, The checklist
incorrectly references a removed pause keybinding; remove or update the stale
items that mention the `pause` key from the checklist entries that list `keyMap`
bindings and the Update wiring (items mentioning `p`, `pause`, and pause/resume
behavior). Specifically, edit the checklist so `keyMap` only documents the
current bindings (e.g., `copyURL`, `help`, `quit`), and remove any TODOs about
implementing `ShortHelp()`, `FullHelp()` or `Update()` wiring for a `pause`/`p`
binding so the checklist matches the actual `keyMap`, `ShortHelp`, `FullHelp`,
and `Update` behavior.
Init only returned tea.RequestBackgroundColor, so the view never re-rendered and the uptime display stayed frozen at 0m00s.
- Fix terminal corruption: remove errant RestoreTerminal defer and suppress stderr writes while alt-screen TUI is active - Fix Ctrl-C exit code: return 0 when prog.Run errors due to ctx cancel (tea.WithContext causes ErrProgramKilled on signal, not a real error) - Fix phantom in-flight rows on reconnect: appendDelivery deduplicates by ID, updating existing rows in-place instead of appending duplicates - Fix stuck in-flight rows: forwardOneTUI sends intermediate DeliveryCompletedMsg per retry attempt so the TUI reflects progress - Fix stale HTTP status in final completion: reset finalStatus each iteration so transport errors don't carry over a prior status code - Fix phantom viewport row: fixedHeaderRows(termH, hasEmail) accounts for the 3-row identity section when email is absent (was always 4) - Surface malformed SSE events as TUI rows with Suffix="malformed" instead of silently dropping them - Add tea.WithContext(ctx) to tie program lifetime to parent context - Log saveCursor write failures in non-TUI mode; suppress in TUI mode to avoid corrupting the alt-screen display - Include actual clipboard error in copyURLCmd failure toast - Add suffix constants (deliverySuffixMalformed, etc.) and use http.MethodPost instead of "POST" string literals - Add tests: forwardOneTUI malformed payload, non-2xx retry, transport-error retry, appendDelivery dedup+eviction interaction - Fix .gitignore: anchor hooksctl rule to root (was matching cmd/hooksctl/)
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
hooksctl forwardwhen stdout is a TTY: live delivery log with status, latency, size, source, uptime, session state (online / reconnecting / offline), copy-URL keybind, and a help overlayTest plan
make test— full suite passes with-racemake lint— 0 issuesTestForwardOneTUI_MalformedPayload— target not reached; TUI row showsmalformedsuffix;errSkipEventreturnedTestForwardOneTUI_RetryOnNonSuccess— intermediateretryingrow shown for 503; final row shows 200TestForwardOneTUI_RetryOnTransportError— hijacked connection producestransport errrow; final row shows 200TestAppendDelivery_DeduplicatesID— reconnect re-deliver updates existing row in-placeTestAppendDelivery_DedupRunsBeforeEviction— dedup check runs before ring-eviction so re-appending at capacity doesn't lose the oldest unrelated entryTestFixedHeaderRows— 4-row identity with email, 3-row without, 2-row compactReview notes
InFlight=trueis intentional — the event is being retried. The cursor is only advanced after a successfulforwardOneTUI, so a completed-200 row can never be re-delivered.prog.Run()return a non-context error from a test double; deferred for a future integration harness.clipboard.WriteAllcan't be easily injected. Low priority.