feat: Add interactive listen mode #159
Conversation
|
Hi @alexbouchardd, appreciate the initiative and great work. Still testing it out, gonna track this document for a few issues or DX considerations. Testing done in iTerm, using zsh. Open in dashboardThis is not related to this but worth a note I think. The first hiccup I ran into was that my CLI is using a different project from the one on the dashboard. So when it opens the event link, because the URL doesn't contain the project identifier, I got stuck and it didn't load. Issues rendering & navigating eventsCleanShot.2025-10-09.at.11.37.56.mp4I discovered the issue was related to window/pane size. This issue only happened when I was in split pane, or when I set the terminal window at half-screen. One follow-up is that the rendering is based on the view port size. It may lead to poor UI when the viewport changes (change number of pane or change terminal window size from full screen to half screen or vice versa). 
We can consider using a dedicated package for TUI rendering such as bubbletea or tview. UX: Layout shift while navigating event listCleanShot.2025-10-09.at.16.25.47.mp4Because we're adding the caret An idea is to allocate whitespace for the caret, and if we'd like to make the line stand out then maybe we can slightly dim the unselected events to keep the selected line stand out better. Just a UX thing, not a huge concern. UX: Exit event data screen inconsistencyMy understanding is we can CleanShot.2025-10-09.at.16.32.13.mp4UX: terminal historyI started a new terminal tab, started the CLI, played around with it, then exited. It left a pretty poor terminal state afterwards, taking over the history of the terminal before starting the session. It's not a big deal, but I think a better approach of running the session in an "alternate canvas", similar to the Show Data UI, would be a better DX. CleanShot.2025-10-09.at.16.43.58.mp4
Overall I really like the new experience! Amazing job on leading this effort. The ability to inspect & retry is very nice. I think the main concern is the rendering aspect. If this experience works consistently then it's a great experience. But I'm not 100% sure if it will work across the board yet. Because of that, I think it may make sense if we make this an opt-in experience via As for the PR, I only skimmed the code and asked Claude to review. Nothing stood out, and things seem to work correctly. Happy to move it forward and I personally would love to spend more time on this myself when I have the chance. |
|
As per @alexluong comment, should the team ID param be present in all links to ensure the correct project is opened? Note: I still think the org and project should be in the URL path. |
I concur. |
|
PR review from Claude, I think some of these can be valid PR Review: Interactive Keyboard Shortcuts and Output Mode ControlsSummaryThis PR adds interactive keyboard shortcuts and output mode controls to the Overall AssessmentArchitecture: ✅ Good separation of concerns Verdict: Solid foundation, but needs hardening before production use. The critical issues should be fixed before merging. Critical Issues 🔴1. Race Condition in Event SelectionLocation: func (eh *EventHistory) GetSelectedEvent() *WebhookEvent {
eh.mu.RLock()
defer eh.mu.RUnlock()
if eh.selectedIndex >= len(eh.events) {
return nil
}
return eh.events[eh.selectedIndex] // ❌ Index could be stale
}Problem: Between checking the length and accessing the array, events could be added/removed by another goroutine. While you have a lock, Scenario:
Fix: Add bounds checking: if eh.selectedIndex < 0 || eh.selectedIndex >= len(eh.events) {
eh.selectedIndex = max(0, len(eh.events)-1) // Reset to valid
}2. Terminal State Not Restored on PanicLocation: func (ui *TerminalUI) EnableRawMode() error {
oldState, err := term.MakeRaw(int(os.Stdin.Fd()))
if err != nil {
return err
}
ui.oldState = oldState
return nil
}Problem: If the program panics while in raw mode, the terminal is left broken (no echo, weird input handling). User has to type Fix: Add a defer in the main proxy Run function: func (p *Proxy) Run(ctx context.Context) error {
if p.cfg.Output == "interactive" {
p.ui.EnableRawMode()
defer func() {
if p.ui.oldState != nil {
term.Restore(int(os.Stdin.Fd()), p.ui.oldState)
}
}()
}
// ... rest of code
}3. Context Cancellation Doesn't Stop Keyboard ListenerLocation: go func() {
for {
select {
case <-ctx.Done():
return
default:
r, _, err := reader.ReadRune() // ❌ BLOCKS HERE
// ...
}
}
}()Problem: When context is cancelled, the goroutine checks Impact: Program won't exit cleanly when user presses Ctrl+C until they press another key. Fix: This is hard to fix properly without closing stdin or using a timeout. One approach: go func() {
for {
// Non-blocking channel check
select {
case <-ctx.Done():
return
default:
}
// Set read deadline (requires *os.File, not bufio)
os.Stdin.SetReadDeadline(time.Now().Add(100 * time.Millisecond))
r, _, err := reader.ReadRune()
if err != nil {
if errors.Is(err, os.ErrDeadlineExceeded) {
continue // Timeout, check ctx.Done() again
}
return
}
// ... handle key
}
}()4. Terminal Resize Handling (Line Wrapping)Location: Problem: When terminal width changes, event lines wrap to multiple terminal lines, but the viewport calculation assumes one event = one line. This causes overlapping text and broken rendering. Issues:
Fix:
func formatEventLine(event *WebhookEvent, isSelected bool, maxWidth int) string {
// ... build line as before ...
// Strip ANSI codes to measure actual display width
displayWidth := len(ansi.StripANSI(line))
if displayWidth > maxWidth {
// Truncate and add ellipsis
line = truncateWithANSI(line, maxWidth-3) + "..."
}
return line
}
import "os/signal"
import "syscall"
sigwinch := make(chan os.Signal, 1)
signal.Notify(sigwinch, syscall.SIGWINCH)
go func() {
for {
select {
case <-ctx.Done():
return
case <-sigwinch:
// Terminal was resized - trigger re-render
ui.triggerRerender()
}
}
}()High Priority Issues 🟡5. HTTP Response Not Checked in RetryLocation: resp, err := client.Post(context.Background(), retryURL, []byte("{}"), nil)
if err != nil {
// ... log error
return
}
defer resp.Body.Close()
// ❌ No check if resp.StatusCode >= 400Problem: A 404 or 500 response is treated as success - no feedback to user. Fix: if err != nil || resp.StatusCode >= 400 {
ea.ui.SafePrintf("[%s] Retry failed (status %d)\n",
color.Red("ERROR").Bold(), resp.StatusCode)
return
}
ea.ui.SafePrintf("[%s] Event retried successfully\n",
color.Green("✓"))6. No Feedback After ActionsLocation: When user presses Fix: Print status messages via 7. Temporary File Cleanup RaceLocation: tmpfile, err := os.CreateTemp("", "hookdeck-event-*.txt")
// ...
defer os.Remove(tmpfile.Name()) // ❌ Removed too early
// ...
cmd := exec.Command("less", "-R", "-P?eEND:.", tmpfile.Name())
cmd.Stdout = tty
cmd.Stdin = tty
cmd.Stderr = tty
err = cmd.Run() // ❌ File might not exist anymore!Problem: Fix: Remove file after tmpfile, err := os.CreateTemp("", "hookdeck-event-*.txt")
// ... write content, close file
cmd := exec.Command("less", "-R", "-P?eEND:.", tmpfile.Name())
// ... setup stdin/stdout/stderr
err = cmd.Run()
os.Remove(tmpfile.Name()) // Now safe to remove8. No Maximum Event History LimitLocation: func (eh *EventHistory) AddEvent(event *WebhookEvent) {
eh.mu.Lock()
defer eh.mu.Unlock()
eh.events = append(eh.events, event) // ❌ Unbounded growth
eh.selectedIndex = len(eh.events) - 1
}Problem: In a long-running session with thousands of events, memory grows indefinitely. Fix: Keep only last N events: const maxEventHistory = 100
func (eh *EventHistory) AddEvent(event *WebhookEvent) {
eh.mu.Lock()
defer eh.mu.Unlock()
eh.events = append(eh.events, event)
// Prune old events
if len(eh.events) > maxEventHistory {
eh.events = eh.events[len(eh.events)-maxEventHistory:]
// Adjust selectedIndex if needed
if eh.selectedIndex >= len(eh.events) {
eh.selectedIndex = len(eh.events) - 1
}
}
eh.selectedIndex = len(eh.events) - 1
}Medium Priority Issues 🟢9. Error Handling in Browser OpeningLocation: User gets an error log but no actionable message. Should print the URL so they can open it manually. Fix: if err := exec.Command(cmd, args...).Start(); err != nil {
ea.ui.SafePrintf("[%s] Couldn't open browser. Visit: %s\n",
color.Yellow("WARN"), eventURL)
return
}10. JSON Unmarshaling Silently FailsLocation: If headers aren't valid JSON, they're just not shown. Should fall back to raw display. Fix: if err := json.Unmarshal(...); err == nil {
// Pretty print
} else {
content.WriteString("(raw) " + string(webhookEvent.Body.Request.Headers))
}11. No Validation of Event DataLocation:
Fix: Add nil checks before accessing nested fields. 12. Platform-Specific Code Without Graceful FallbackLocation: tty, err := os.OpenFile("/dev/tty", os.O_RDWR, 0)This is Unix/Linux/macOS only. On Windows, 13. Status Line Calculation Doesn't Account for Long MessagesLocation: func (ui *TerminalUI) updateStatusLine(message string) {
width, height, _ := term.GetSize(int(os.Stdin.Fd()))
fmt.Fprintf(os.Stdout, "\033[%d;0H", height)
// ... prints message
}If 14. Pollutes Scrollback BufferThe application leaves rendering artifacts in terminal history after exit. This makes it difficult to see what was in the terminal before the CLI session. Better behavior: Use alternate screen buffer (like // On startup
fmt.Print("\x1b[?1049h") // Switch to alternate screen
fmt.Print("\x1b[?25l") // Hide cursor
// On exit
fmt.Print("\x1b[?25h") // Show cursor
fmt.Print("\x1b[?1049l") // Return to main screenDesign/Logic Issues 🔵15. selectedIndex Always Points to Latest EventLocation: func (eh *EventHistory) AddEvent(event *WebhookEvent) {
// ...
eh.selectedIndex = len(eh.events) - 1 // ❌ Moves selection
}UX Problem: User navigates to event #5 to inspect it. New event arrives. Selection jumps to latest event. User loses their place. Better UX: Keep selection on the event user chose, unless they explicitly move it or the selected event is pruned from history. 16. No Visual Indicator of Scroll PositionWhen there are 100 events but only 10 visible, user has no idea where they are in the list (e.g., "showing 50-60 of 100"). Suggestion: Add to status line: 17. Keyboard Shortcuts Not DiscoverableIf user misses the status line or it scrolls away, they don't know what keys do what. Suggestion: Add help screen ( 18. Connection Creation Message Not in Interactive FlowLocation: fmt.Printf("\nThere's no CLI destination connected to %s, creating one named %s\n", ...)This prints during setup, before interactive mode starts. In interactive mode, it clutters the event area. Code Quality Issues 📝19. Magic Numbers
Should be named constants. 20. Duplicated Terminal Size Queries
21. No TestsFor a complex interactive feature, there should be unit tests for:
RecommendationsMust Fix Before Merge:
Should Fix Before Merge:
Nice to Have:
Testing RecommendationsSince this is a large change affecting terminal I/O, test:
Positive Highlights
|
No description provided.