DevMode: improvements around feedback #300
Conversation
WalkthroughThe changes enhance agent metadata retrieval and HTTP response handling in the development server, introducing richer agent information with welcome messages and retry logic. The TUI log system is refactored to use structured log items with timestamps, improves log display formatting, and adds mouse-based log navigation and selection. GitHub Actions workflows were updated to ignore documentation-only changes on pull requests. Port selection logic was refined by adding an optional port hint parameter and changing port availability checks. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Server
participant LocalWelcomeEndpoint
participant Project
User->>Server: GET /_agents
Server->>Project: Fetch agent configs
Server->>LocalWelcomeEndpoint: GET /welcome (with retries)
LocalWelcomeEndpoint-->>Server: Welcome messages (or 404)
Server->>User: Respond with enriched agent data (configs + welcome)
sequenceDiagram
participant User
participant TUI
participant LogSource
LogSource->>TUI: AddLog(message)
TUI->>TUI: Create logItem (timestamp, message)
TUI->>TUI: Append logItem to log list
User->>TUI: Mouse scroll/click or key event
TUI->>TUI: Update selection/view accordingly
User->>TUI: Select log entry
TUI->>User: Show log modal (timestamp + message)
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🔇 Additional comments (11)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
internal/dev/tui.go (1)
67-70: Redundantstring()conversions – can be removed
messageis already astring, so the explicitstring(i.message)casts inTitleandFilterValueallocate unnecessarily. Removing them avoids the extra (albeit small) allocation and keeps the code cleaner.-func (i logItem) Title() string { return strings.ReplaceAll(string(i.message), "\n", " ") } -... -func (i logItem) FilterValue() string { return string(i.message) } +func (i logItem) Title() string { return strings.ReplaceAll(i.message, "\n", " ") } +... +func (i logItem) FilterValue() string { return i.message }internal/dev/server.go (2)
430-456: Resource leak & unnecessarydeferin retry loopUsing
defer resp.Body.Close()inside a loop means only the last response body is closed; earlier ones remain open until the function returns.
With up to five retries this is minor but unnecessary – and easy to fix.- resp, err := http.DefaultClient.Do(req) + resp, err := http.DefaultClient.Do(req) … - defer resp.Body.Close() + // ensure each attempt is closed promptly + defer resp.Body.Close()Alternatively, close explicitly at the bottom of the loop and remove the
defer.Also, consider exposing the back-off parameters (maxAttempts, baseDelay) as constants for easier tuning/testing.
409-420: Improve retry‐able error detectionString matching on error messages is brittle and locale-dependent.
Useerrors.Asto detect*net.OpError,syscall.Errno == ECONNREFUSED|ECONNRESET, etc., for a more future-proof check.Not critical but will avoid edge cases on different platforms or Go versions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
internal/dev/server.go(7 hunks)internal/dev/tui.go(10 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
internal/dev/server.go (1)
internal/bundler/bundler.go (1)
AgentConfig(28-32)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build and Test (windows-latest)
- GitHub Check: Test CLI Upgrade Path (windows-latest)
- GitHub Check: Analyze (go)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
internal/dev/server.go (1)
272-278:⚠️ Potential issueHTTP protocol violation: multiple WriteHeader calls
This code will cause a runtime panic because you're calling
WriteHeaderafter headers have already been sent on line 268 withw.WriteHeader(http.StatusOK).Apply this fix to avoid the HTTP protocol violation:
agents, err := s.getAgents(r.Context(), s.Project.Project) if err != nil { s.logger.Error("failed to marshal agents control response: %s", err) - w.WriteHeader(http.StatusInternalServerError) + w.Write([]byte(fmt.Sprintf("event: error\ndata: %q\n\n", err.Error()))) + rc.Flush() return }This keeps the connection valid and informs the client of the failure without violating the HTTP contract.
🧹 Nitpick comments (1)
internal/dev/server.go (1)
424-450: Consider exponential backoff for retry mechanismWhile the retry logic is solid, consider using an exponential backoff pattern similar to what's used in the
HealthCheckmethod for more efficient retries.func (s *Server) getWelcome(ctx context.Context, port int) (map[string]Welcome, error) { url := fmt.Sprintf("http://127.0.0.1:%d/welcome", port) for i := 0; i < 5; i++ { req, err := http.NewRequestWithContext(ctx, "GET", url, nil) if err != nil { return nil, err } resp, err := http.DefaultClient.Do(req) if err != nil { if isConnectionErrorRetryable(err) { - time.Sleep(time.Millisecond * time.Duration(100*i+1)) + dur := time.Millisecond * 150 * time.Duration(math.Pow(float64(i+1), 2)) + time.Sleep(dur) continue } return nil, err } defer resp.Body.Close() if resp.StatusCode == 404 { return nil, nil // this is ok, just means no agents have inspect } + if resp.StatusCode != http.StatusOK { + s.logger.Trace("welcome request returned status code: %d", resp.StatusCode) + dur := time.Millisecond * 150 * time.Duration(math.Pow(float64(i+1), 2)) + time.Sleep(dur) + continue + } res := make(map[string]Welcome) if err := json.NewDecoder(resp.Body).Decode(&res); err != nil { return nil, err } return res, nil } return nil, fmt.Errorf("failed to inspect agents after 5 attempts") }This change also adds handling for non-200 and non-404 status codes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
internal/dev/server.go(6 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
internal/dev/server.go (1)
internal/bundler/bundler.go (1)
AgentConfig(28-32)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Test (windows-latest)
🔇 Additional comments (8)
internal/dev/server.go (8)
196-199: Clear struct design with embeddingThe
AgentWelcomestruct is well-designed, embedding the existingAgentConfigfrom the project package and the newWelcomestruct. This approach maintains backward compatibility while extending functionality.
204-204: Appropriate type modificationUpdating the
Agentsfield to use the newAgentWelcometype provides a clean way to include welcome messages with agent configurations.
207-223: Well-structured agent data retrievalThe
getAgentsfunction effectively combines agent configurations with welcome data. The code handles potential errors fromgetWelcomeappropriately and constructs the response in a clean manner.
253-261: Using enhanced agent data in the responseGood refactoring to use the new
getAgentsmethod instead of directly accessing the agents from the project. This allows for the inclusion of welcome messages in the response.
270-270: Good addition of health checkAdding a health check before sending the start event ensures the server is running before attempting to fetch agent data.
306-309: User-friendly GET handler for agent pathsAdding a specific handler for GET requests to agent paths provides a better user experience when users directly access agent URLs in a browser.
403-414: Useful helper for connection error handlingThe
isConnectionErrorRetryablefunction provides a clean way to identify errors that can be retried, with good coverage of different error messages across platforms.
416-422: Well-structured Welcome data modelThe
Welcomestruct provides a clear model for welcome messages and optional prompts with proper JSON field tags.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/go.yml (1)
6-8: Suggest DRY improvement via YAML anchors
To reduce duplication and make future updates easier, consider defining a YAML anchor for the list of documentation files and reusing it under bothpushandpull_request.on: push: branches: [ "main" ] - paths-ignore: - - 'README.md' - - 'CHANGELOG.md' + paths-ignore: &doc-ignore + - 'README.md' + - 'CHANGELOG.md' pull_request: branches: [ "main" ] - paths-ignore: - - 'README.md' - - 'CHANGELOG.md' + paths-ignore: *doc-ignoreAlso applies to: 11-13
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
.github/workflows/go.yml(1 hunks).github/workflows/upgrade-test.yml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/upgrade-test.yml
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Test (windows-latest)
🔇 Additional comments (1)
.github/workflows/go.yml (1)
11-13: Consistent path-ignore addition
You've addedpaths-ignoreunderpull_requestto mirror the existing configuration forpush. This prevents CI runs on doc-only PRs and aligns with the update inupgrade-test.yml.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/dev/tui.go (1)
477-481: Unbounded log growth still presentThe
AddLogmethod continues to append tom.logItemswithout any limit, which can lead to memory exhaustion in long-running sessions.Consider implementing a retention policy by limiting the number of logs kept in memory, such as:
func (d *DevModeUI) AddLog(log string, args ...any) { + const maxLogs = 1000 // or make this configurable d.program.Send(addLogMsg{ timestamp: time.Now(), message: ansiColorStripper.ReplaceAllString(fmt.Sprintf(log, args...), ""), }) + // Trim logs if we exceed the maximum (will need to be done in the model's Update method) + if len(d.model.logItems) > maxLogs { + d.model.logItems = d.model.logItems[len(d.model.logItems)-maxLogs:] + d.program.Send(tea.Cmd(func() tea.Msg { + return d.model.logList.SetItems(d.model.logItems) + })) + } }
🧹 Nitpick comments (2)
internal/dev/server.go (2)
404-415: Consider using error type checking rather than string matching.The function uses string matching to identify retryable errors, which can be fragile if error messages change.
Consider using error type checking when possible, or at least define constants for the error strings:
+const ( + errConnectionRefused = "connection refused" + errConnectionReset = "connection reset by peer" + errWindowsRefused = "No connection could be made because the target machine actively refused it" +) func isConnectionErrorRetryable(err error) bool { - if strings.Contains(err.Error(), "connection refused") { + if strings.Contains(err.Error(), errConnectionRefused) { return true } - if strings.Contains(err.Error(), "connection reset by peer") { + if strings.Contains(err.Error(), errConnectionReset) { return true } - if strings.Contains(err.Error(), "No connection could be made because the target machine actively refused it") { // windows + if strings.Contains(err.Error(), errWindowsRefused) { // windows return true } return false }
425-451: Consider expanding retry logic in getWelcome.The method only retries on connection errors, but other transient errors might also benefit from retries.
func (s *Server) getWelcome(ctx context.Context, port int) (map[string]Welcome, error) { url := fmt.Sprintf("http://127.0.0.1:%d/welcome", port) for i := 0; i < 5; i++ { req, err := http.NewRequestWithContext(ctx, "GET", url, nil) if err != nil { return nil, err } resp, err := http.DefaultClient.Do(req) if err != nil { - if isConnectionErrorRetryable(err) { + // Retry on connection errors and context deadline exceeded + if isConnectionErrorRetryable(err) || errors.Is(err, context.DeadlineExceeded) { time.Sleep(time.Millisecond * time.Duration(100*i+1)) continue } return nil, err } defer resp.Body.Close() if resp.StatusCode == 404 { return nil, nil // this is ok, just means no agents have inspect } + // Retry on server errors (5xx) + if resp.StatusCode >= 500 { + time.Sleep(time.Millisecond * time.Duration(100*i+1)) + continue + } res := make(map[string]Welcome) if err := json.NewDecoder(resp.Body).Decode(&res); err != nil { + // Retry on JSON decode errors + time.Sleep(time.Millisecond * time.Duration(100*i+1)) + continue } return res, nil } return nil, fmt.Errorf("failed to inspect agents after 5 attempts") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
internal/dev/server.go(6 hunks)internal/dev/tui.go(10 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
internal/dev/server.go (1)
internal/bundler/bundler.go (1)
AgentConfig(28-32)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build and Test (windows-latest)
- GitHub Check: Test CLI Upgrade Path (blacksmith-4vcpu-ubuntu-2204)
- GitHub Check: Analyze (go)
🔇 Additional comments (10)
internal/dev/server.go (4)
196-199: Good addition of the AgentWelcome struct.The new
AgentWelcomestruct appropriately embeds the existingproject.AgentConfigand adds the newWelcomefield to enhance agent information.
252-260: Agent endpoint implementation looks good.Updated code now correctly uses the new
getAgentsmethod for retrieving enriched agent data including welcome messages.
270-280: Fixed WriteHeader issue correctly.The code now properly handles errors from
getAgentsby writing an SSE-style error event instead of attempting to callWriteHeaderafter headers have been sent, addressing the issue mentioned in the previous review.
307-310: Enhanced agent path handling for GET requests.Added a special case for GET requests to agent paths, providing a more user-friendly response when users navigate to these URLs in a browser.
internal/dev/tui.go (6)
62-69: Good enhancement to logItem structure.Converting
logItemfrom a string alias to a struct with timestamp and message fields improves the log structure and enables better formatting and filtering capabilities.
96-96: Improved selected item visibility.Using
selectedColorand making selected items bold improves visual distinction of the currently selected log entry.
203-220: Excellent mouse support implementation.The mouse event handling correctly updates the list model first before checking the selected item, which fixes the issue raised in the previous review where clicking would select the wrong item.
291-295: Better log ordering and tracking.Logs are now displayed in reverse chronological order and auto-scroll to follow new entries when not paused, providing a better user experience for following live logs.
352-352: Enhanced log display with timestamps.Adding timestamp information to the selected log display provides valuable context for log entries.
456-456: Full mouse motion support enabled.Added full mouse motion support which complements the mouse event handling implementation.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/dev/server.go (2)
408-419: Consider adding a more comprehensive connection error check.The function checks for some common connection errors but might miss other retryable network errors like timeout errors or temporary DNS resolution failures.
func isConnectionErrorRetryable(err error) bool { if strings.Contains(err.Error(), "connection refused") { return true } if strings.Contains(err.Error(), "connection reset by peer") { return true } if strings.Contains(err.Error(), "No connection could be made because the target machine actively refused it") { // windows return true } + // Add checks for timeouts, temporary DNS failures + if strings.Contains(err.Error(), "i/o timeout") { + return true + } + if strings.Contains(err.Error(), "temporary failure in name resolution") { + return true + } + // Check for net.Error with Temporary() or Timeout() methods + var netErr net.Error + if errors.As(err, &netErr) { + return netErr.Temporary() || netErr.Timeout() + } return false }
429-455: Consider using exponential backoff for retry logic.The current implementation uses a linear backoff strategy (100ms * i), while the
HealthCheckmethod uses exponential backoff. For consistency and better handling of transient issues, consider using exponential backoff here as well.func (s *Server) getWelcome(ctx context.Context, port int) (map[string]Welcome, error) { url := fmt.Sprintf("http://127.0.0.1:%d/welcome", port) for i := 0; i < 5; i++ { req, err := http.NewRequestWithContext(ctx, "GET", url, nil) if err != nil { return nil, err } resp, err := http.DefaultClient.Do(req) if err != nil { if isConnectionErrorRetryable(err) { - time.Sleep(time.Millisecond * time.Duration(100*i+1)) + dur := time.Millisecond * 150 * time.Duration(math.Pow(float64(i+1), 2)) + time.Sleep(dur) continue } return nil, err } defer resp.Body.Close() if resp.StatusCode == 404 { return nil, nil // this is ok, just means no agents have inspect } res := make(map[string]Welcome) if err := json.NewDecoder(resp.Body).Decode(&res); err != nil { return nil, err } return res, nil } return nil, fmt.Errorf("failed to inspect agents after 5 attempts") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
internal/dev/server.go(6 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
internal/dev/server.go (1)
internal/bundler/bundler.go (1)
AgentConfig(28-32)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Build and Test (windows-latest)
- GitHub Check: Analyze (go)
- GitHub Check: Test CLI Upgrade Path (windows-latest)
- GitHub Check: Build and Test (blacksmith-4vcpu-ubuntu-2204)
- GitHub Check: Test CLI Upgrade Path (blacksmith-4vcpu-ubuntu-2204)
🔇 Additional comments (5)
internal/dev/server.go (5)
196-199: Well-designed struct embedding for agent metadata enrichment.The new
AgentWelcomestruct extends the base agent data with welcome information by embedding both theproject.AgentConfigandWelcomestructs, which is a clean approach to enhancing the agent response data.
201-205: Good improvement to response structure.The updated
AgentsControlResponsestruct now contains richer agent information through theAgentWelcometype, providing more context to clients.
207-227: Fixed the potential nil pointer dereference issue.The implementation correctly checks if the
welcomemap is nil before accessing it, addressing the issue identified in the previous review.for _, agent := range project.Agents { var w Welcome if welcome != nil { w = welcome[agent.ID] } resp.Agents = append(resp.Agents, AgentWelcome{ AgentConfig: agent, Welcome: w, }) }
276-282: Fixed the WriteHeader issue with SSE-style error event.The code now correctly sends an SSE-style error event instead of calling
WriteHeaderafter headers have been sent, resolving the issue raised in the previous review.agents, err := s.getAgents(r.Context(), s.Project.Project) if err != nil { s.logger.Error("failed to marshal agents control response: %s", err) - w.WriteHeader(http.StatusInternalServerError) + w.Write([]byte(fmt.Sprintf("event: error\ndata: %q\n\n", err.Error()))) + rc.Flush() return }
274-274: Good practice to verify server readiness.Adding the health check before sending the start event ensures the server is up and ready to handle requests, improving reliability.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
internal/dev/tui.go (1)
292-296: 🛠️ Refactor suggestionPotential unbounded log growth
The
AddLogfunction keeps appending tom.logItemswithout any limit, which can cause memory exhaustion in long-running sessions. This issue was previously identified in a review but still exists.Consider implementing a log retention policy that limits the number of stored logs:
m.logItems = append(m.logItems, logItem(msg)) +// Keep only the last N log items to prevent memory exhaustion +const maxLogItems = 1000 // Adjust this number as needed +if len(m.logItems) > maxLogItems { + m.logItems = m.logItems[len(m.logItems)-maxLogItems:] +} cmd = append(cmd, m.logList.SetItems(m.logItems))
🧹 Nitpick comments (1)
internal/dev/tui.go (1)
451-451: Add type assertion error handlingType assertions like
item.(logItem)don't check if the assertion succeeded, which could potentially cause a panic.-fmt.Println(item.(logItem).raw) +if logItem, ok := item.(logItem); ok { + fmt.Println(logItem.raw) +} else { + fmt.Println("Error: unexpected log item type") +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
internal/dev/tui.go(10 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build and Test (windows-latest)
- GitHub Check: Test CLI Upgrade Path (windows-latest)
- GitHub Check: Build and Test (blacksmith-4vcpu-ubuntu-2204)
- GitHub Check: Analyze (go)
🔇 Additional comments (11)
internal/dev/tui.go (11)
28-28: Improved selected item visibility with custom colorThe introduction of a dedicated
selectedColorenhances the UI by providing better visual distinction for the currently selected log entry.
62-66: Good enhancement: Structured log items with timestampsConverting log items from simple strings to a structured type with timestamp, message, and raw text significantly improves the functionality of the log system. This change enables additional features like displaying timestamps when viewing log details.
68-70: Log item methods updated for new structureThe methods have been appropriately adapted to work with the new structured log format.
97-97: Improved selected item stylingThe styling for selected items now uses the new
selectedColorand removes the left border, creating better visual distinction.
182-182: UI label renamed from "Dashboard" to "DevMode"This change aligns with the PR objective to rename the application label.
204-221: Well-implemented mouse interaction supportThe mouse event handling correctly implements wheel scrolling and item selection. The implementation properly addresses the previous review comment about selecting the wrong item on left-click by first updating the list model before accessing the selected item.
227-229: Improved key handling for filter mode and Escape keyThe code now properly handles key events when in filter mode and explicitly manages Escape key behavior when not closing modals, improving the overall user experience.
Also applies to: 244-244
353-353: Enhanced log detail view with timestampsThe log modal view now displays both timestamp and formatted message, making better use of the new structured log data.
449-452: Improved session exit behaviorThe Close method now properly clears the screen and preserves content by printing all raw log lines on exit, aligning with the PR objective of not losing content when exiting.
461-461: Added full mouse motion supportThe program now uses
tea.WithMouseAllMotion()instead oftea.WithAltScreen(), which enables mouse interactions while preserving content on exit.
478-483: Enhanced log message creation with timestampsThe
AddLogmethod now creates a structured log item with timestamp, raw message, and ANSI-stripped message, providing richer log information.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/dev/tui.go (1)
482-488: Potential unbounded log growthThe
AddLogfunction keeps appending tom.logItemswithout any limit, which could lead to memory issues in long-running sessions.Consider implementing a log retention policy by trimming old logs after a certain threshold is reached:
func (d *DevModeUI) AddLog(log string, args ...any) { raw := fmt.Sprintf(log, args...) + // Limit log history to avoid memory exhaustion + const maxLogs = 1000 + if len(d.model.logItems) >= maxLogs { + // Remove oldest logs when limit is reached + d.model.logItems = d.model.logItems[len(d.model.logItems)-maxLogs+1:] + } d.program.Send(addLogMsg{ id: uuid.New().String(), timestamp: time.Now(), raw: raw, message: strings.ReplaceAll(ansiColorStripper.ReplaceAllString(raw, ""), "\n", " "), }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
go.mod(3 hunks)internal/dev/tui.go(12 hunks)
✅ Files skipped from review due to trivial changes (1)
- go.mod
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build and Test (windows-latest)
- GitHub Check: Test CLI Upgrade Path (windows-latest)
🔇 Additional comments (13)
internal/dev/tui.go (13)
18-19: Added dependencies for enhanced UI interactionsThese new imports support the UUID generation for log tracking and the bubblezone library for mouse interaction zones, which are key components for the new mouse support feature.
30-30: LGTM - Improved highlight color for selected logsThe new
selectedColorprovides better visibility for selected log items, meeting the PR objective for improved visual feedback.
64-69: LGTM - Enhanced log item structureThe transformed
logItemtype now includes essential fields like ID, timestamp, and separate raw/processed messages, supporting richer log display and interaction features.
71-71: LGTM - Added mouse interaction supportUsing
zone.Mark()enables clickable zones for log items, which is essential for the mouse interaction features described in the PR objectives.Also applies to: 73-73
76-76: LGTM - Updated message typeThe
addLogMsgtype update properly reflects the newlogItemstructure.
100-100: LGTM - Improved selected item visibilityUsing the new
selectedColorfor the selected title enhances visual feedback when navigating logs.
230-232: LGTM - Improved filter mode handlingBreaking early when in filter mode ensures that key events are correctly delegated to the list's filter handling.
247-247: LGTM - Fixed escape key behaviorThe explicit handling of escape key presses that don't close modals addresses the PR objective of fixing escape key behavior.
295-299: LGTM - Logs shown in reverse order with auto-trackingThe implementation now appends new logs to the end and automatically selects the newest log when not paused, meeting the PR objective for log display order and tracking.
356-356: LGTM - Added timestamp to selected logsThe log modal now displays the formatted timestamp alongside the message, addressing the PR objective of showing timestamps on selected logs.
396-396: LGTM - Added mouse zone trackingWrapping the view with
zone.Scan()enables the mouse tracking features throughout the TUI.
452-455: LGTM - Improved content preservation on exitClearing the screen and printing all raw log lines on exit addresses the PR objective of disabling alternate screen mode to preserve content.
461-461: LGTM - Configured for mouse interaction supportInitializing bubblezone and enabling full mouse motion support are necessary configurations for the mouse interaction features.
Also applies to: 465-465
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
internal/dev/tui.go (3)
216-237:⚠️ Potential issuePotential index out of bounds error in mouse click handling.
When selecting an item on mouse click, the code subtracts 1 from the index (
index := i - 1), which could cause an out of bounds error when clicking the first item (index 0). Although there's a check for negative values after, it's better to fix the root issue.- m.logList.Select(index) + m.logList.Select(i)
226-232:⚠️ Potential issueLeft-click opens the previous selection.
The click handling currently processes the click before letting the list component handle the event. This means that if a user clicks on a row that is not currently selected, the modal will open for the old selection, not the clicked one.
First let the list component process the click:
if zone.Get(v.id).InBounds(msg) { - index := i - 1 - if index < 0 { - index = 0 - } - m.logList.Select(index) + // Let the list update first so the clicked row becomes selected + lm, _ := m.logList.Update(msg) + m.logList = lm + + // Now if we have a selection, set it as the selected log + if sel := m.logList.SelectedItem(); sel != nil { + if log, ok := sel.(logItem); ok { + m.selectedLog = &log + } + } break }
308-312:⚠️ Potential issuePotential unbounded log growth.
AddLogkeeps appending tom.logItemswithout any limit, which can lead to memory exhaustion in long-running sessions.Consider implementing a log rotation mechanism to limit memory usage:
func (m *model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { // ... existing code ... case addLogMsg: + // Keep only the last N logs to prevent memory exhaustion + const maxLogs = 1000 m.logItems = append(m.logItems, logItem(msg)) + if len(m.logItems) > maxLogs { + m.logItems = m.logItems[len(m.logItems)-maxLogs:] + } cmd = append(cmd, m.logList.SetItems(m.logItems)) // ... existing code ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
internal/dev/tui.go(17 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Test (windows-latest)
🔇 Additional comments (14)
internal/dev/tui.go (14)
18-19: Added new dependencies for enhanced UI functionality.The addition of the UUID library and bubblezone package improves the TUI by providing unique identifiers for log items and enabling mouse zone tracking for interactive elements.
30-30: Good improvement for visual distinction.The new
selectedColorenhances the UI by providing a clearer highlight for selected items compared to the previouslabelColor.
64-69: Good enhancement to logItem structure.Converting logItem from a string alias to a struct with ID, timestamp, message, and raw fields provides more structured data that allows for better filtering, sorting, and display formatting.
71-71: Properly implementing mouse zone tracking.Using
zone.Mark()in the Title and FilterValue methods correctly wraps the message content with zone identifiers needed for mouse interaction support.Also applies to: 73-73
160-199: Good refactoring of info box generation.Extracting the info box generation logic into a standalone function improves code organization and reusability, allowing its use in both the model and in non-TUI modes.
189-189: Improved label clarity with "DevMode" instead of "Dashboard".The label change provides better context about what the interface represents.
243-260: Improved key handling logic.The changes to properly ignore keys when in filter mode and to explicitly handle escape key presses improves the user experience by making the navigation more predictable.
369-369: Enhanced log display with timestamp.Adding timestamp formatting to the log modal view improves the user experience by providing temporal context for log entries.
409-409: Properly wrapping view with zone.Scan.Using
zone.Scanto wrap the entire view ensures that all mouse zones are properly tracked and responsive.
444-446: Good fallback detection for VS Code terminal.The addition of VS Code terminal detection helps ensure a better user experience by falling back to non-TUI mode in environments where TUI might not work well.
450-456: Appropriate TUI disabling logic.The code properly checks for TTY availability and VS Code terminal before initializing the TUI model, preventing potential issues in unsupported environments.
506-509: Bubblezone initialization order.The initialization of bubblezone global state is done after checking if TUI is enabled, which is correct. However, ensure that all zone-related functions are only called when TUI is enabled.
530-540: Properly handling log in non-TUI mode.The code correctly checks if TUI is enabled before sending logs to the TUI program, falling back to direct console output in non-TUI mode.
513-513: Enhanced mouse support for better interaction.Adding
tea.WithMouseAllMotion()provides better mouse support, which is essential for the new mouse interaction features.
Summary by CodeRabbit
New Features
Improvements
Chores