Add interactive feedback to remote builds

[deviantony]

No description provided.

[Copilot]

Pull request overview

This PR adds interactive feedback capabilities for remote Docker builds by introducing a passive TUI dashboard that monitors parallel build progress in real-time. The implementation leverages the existing Charmbracelet bubbletea/bubbles libraries to display build status, progress bars, and live logs for multiple services without requiring user interaction.

• Implements a new BuildDashboard TUI component with automatic status updates and progress tracking
• Adds DashboardBuildLogger that integrates with the existing BuildLogger interface
• Provides comprehensive documentation and integration examples for adopting the dashboard in build commands

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 9 comments.

File	Description
internal/build/dashboard.go	Core implementation of passive TUI dashboard with build status tracking, progress bars, and real-time log display
internal/build/dashboard_integration_example.go	Integration examples showing how to use the dashboard with BuildOrchestrator in deploy commands
example_simple_progress.go	Alternative simpler approach using inline progress bars without full TUI
INTERACTIVE_BUILDS_GUIDE.md	Comprehensive guide covering implementation options, integration steps, and troubleshooting

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Race condition: dashboardLogger.MarkServiceQueued() is called before dashboard.Start(), but this will call UpdateService() which attempts to send messages to bd.program. Since bd.program is only initialized in Start() at line 43, these early calls to MarkServiceQueued will silently fail (the nil check in UpdateService prevents a panic but the updates are lost). Move the queued marking calls to after dashboard.Start(), or add a small delay to ensure the dashboard is initialized.

Suggested change

	// Mark all services as queued initially
	for _, name := range serviceNames {
	dashboardLogger.MarkServiceQueued(name)
	}
	// Start dashboard TUI (runs in background)
	dashboard.Start()
	// Start dashboard TUI (runs in background)
	dashboard.Start()
	// Mark all services as queued initially (after dashboard is started)
	for _, name := range serviceNames {
	dashboardLogger.MarkServiceQueued(name)
	}

[Copilot]

The variable name m shadows the map type used in the function. The regex FindStringSubmatch returns a slice of strings (matches), not an error. This code will never work as intended - the condition err != nil will always be false since err is actually a slice.

This should be:

if matches := regexp.MustCompile(`"stream"\s*:\s*"([^"]*)"`)
    .FindStringSubmatch(line); matches != nil && len(matches) > 1 {
    return strings.TrimSpace(matches[1])
}

Suggested change

	var m map[string]interface{}
	if err := regexp.MustCompile(`"stream"\s*:\s*"([^"]*)"`)
	.FindStringSubmatch(line); err != nil && len(err) > 1 {
	return strings.TrimSpace(err[1])
	if matches := regexp.MustCompile(`"stream"\s*:\s*"([^"]*)"`)
	.FindStringSubmatch(line); matches != nil && len(matches) > 1 {
	return strings.TrimSpace(matches[1])

[Copilot]

Creating a new regex compilation on every call to cleanLogLine is inefficient. Consider compiling this regex once as a package-level variable or field in the struct that uses it, similar to how stepRegex is compiled once in NewDashboardBuildLogger.

[Copilot]

The example SetLogger method allows changing the logger on a BuildOrchestrator after initialization, but this could lead to race conditions if builds are already in progress. The logger field in BuildOrchestrator is accessed without synchronization (based on the orchestrator interface). Consider documenting that this should only be called before any build operations start, or implement proper synchronization if runtime logger changes are needed.

Suggested change

	// 1. Add SetLogger method to BuildOrchestrator:
	func (bo *BuildOrchestrator) SetLogger(logger BuildLogger) {
	bo.logger = logger
	}
	// 1. Add SetLogger method to BuildOrchestrator:
	// To make SetLogger thread-safe, add a sync.RWMutex field (e.g., loggerMu) to BuildOrchestrator:
	// type BuildOrchestrator struct {
	// ...
	// logger BuildLogger
	// loggerMu sync.RWMutex
	// ...
	// }
	//
	// Then, update SetLogger to use the mutex:
	func (bo *BuildOrchestrator) SetLogger(logger BuildLogger) {
	bo.loggerMu.Lock()
	defer bo.loggerMu.Unlock()
	bo.logger = logger
	}
	//
	// NOTE: All accesses to bo.logger elsewhere (e.g., in buildService) should also use bo.loggerMu.RLock()/RUnlock()
	// to ensure thread safety.

[Copilot]

Division by zero risk: if total is 0, this will cause a panic. Add a check before calculating the percentage:

if total == 0 {
    return
}
percent := float64(current) / float64(total)

Suggested change

	if total == 0 {
	return
	}

[Copilot]

The documentation references isatty.IsTerminal(os.Stdout.Fd()) but this package is not used elsewhere in the guide. The correct import based on the code examples is golang.org/x/term with term.IsTerminal(int(os.Stdout.Fd())). Update this to match the actual implementation used in the dashboard integration example.

Suggested change

	- Add terminal detection: `isatty.IsTerminal(os.Stdout.Fd())`
	- Add terminal detection: `term.IsTerminal(int(os.Stdout.Fd()))` (import `"golang.org/x/term"`)

[Copilot]

Potential race condition: bd.program is assigned at line 122 and then accessed in a goroutine at line 124, but other methods like UpdateService and Stop also access bd.program without synchronization. If UpdateService or Stop are called immediately after Start() and before the goroutine runs, they might see bd.program as nil or in an inconsistent state. Consider using a mutex to protect access to bd.program or ensuring it's fully initialized before being accessible.

[Copilot]

This statement is unreachable.

[Copilot]

This statement is unreachable.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.

Comments suppressed due to low confidence (1)

internal/build/dashboard.go:327

• The variable m is declared but never used. This declaration should be removed as it serves no purpose and makes the code confusing, especially given the bug in the subsequent if condition.

		var m map[string]interface{}

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

[nitpick] The hardcoded sleep durations (1 second on error, 2 seconds on success) are magic numbers that make the code less maintainable. Consider:

1. Defining these as named constants at the package level (e.g., dashboardErrorDisplayDuration, dashboardSuccessDisplayDuration)
2. Making them configurable if user experience feedback suggests different timing would be better

This would improve code readability and make it easier to adjust these values in the future.

[Copilot]

[nitpick] The hardcoded sleep durations (1 second on error, 2 seconds on success) are magic numbers that make the code less maintainable. Consider:

1. Defining these as named constants at the package level (e.g., dashboardErrorDisplayDuration, dashboardSuccessDisplayDuration)
2. Making them configurable if user experience feedback suggests different timing would be better

This would improve code readability and make it easier to adjust these values in the future.

[Copilot]

Race condition: Reading svc.Status without holding the service's mutex lock. While m.mu.RLock() is held (line 201), individual service fields should be protected by their own mutex (svc.mu) when accessed. The code correctly locks svc.mu later at line 247, but this early access at line 224 is unprotected and could race with updates in the Update() method (line 166).

Consider either:

1. Locking each service mutex before reading Status in this loop
2. Moving the status counting logic to after the individual service locks in the main loop

[Copilot]

[nitpick] The hardcoded sleep durations (1 second on error, 2 seconds on success) are magic numbers that make the code less maintainable. Consider:

1. Defining these as named constants at the package level (e.g., dashboardErrorDisplayDuration, dashboardSuccessDisplayDuration)
2. Making them configurable if user experience feedback suggests different timing would be better

This would improve code readability and make it easier to adjust these values in the future.

[Copilot]

[nitpick] The hardcoded sleep durations (1 second on error, 2 seconds on success) are magic numbers that make the code less maintainable. Consider:

1. Defining these as named constants at the package level (e.g., dashboardErrorDisplayDuration, dashboardSuccessDisplayDuration)
2. Making them configurable if user experience feedback suggests different timing would be better

This would improve code readability and make it easier to adjust these values in the future.

[Copilot]

This statement is unreachable.

Suggested change

	var m map[string]interface{}
	if err := regexp.MustCompile(`"stream"\s*:\s*"([^"]*)"`)
	.FindStringSubmatch(line); err != nil && len(err) > 1 {
	return strings.TrimSpace(err[1])
	matches := regexp.MustCompile(`"stream"\s*:\s*"([^"]*)"`).FindStringSubmatch(line)
	if len(matches) > 1 {
	return strings.TrimSpace(matches[1])

[Copilot]

This statement is unreachable.

Suggested change

	if err := regexp.MustCompile(`"stream"\s*:\s*"([^"]*)"`)
	.FindStringSubmatch(line); err != nil && len(err) > 1 {
	return strings.TrimSpace(err[1])
	if matches := regexp.MustCompile(`"stream"\s*:\s*"([^"]*)"`)
	.FindStringSubmatch(line); matches != nil && len(matches) > 1 {
	return strings.TrimSpace(matches[1])

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The dashboard initialization and lifecycle management logic (lines 104-139) is duplicated between cmd/deploy/deploy.go and cmd/redeploy/redeploy.go. Consider extracting this logic into a shared helper function in the internal/build package to reduce code duplication and improve maintainability. For example:

// In internal/build/dashboard.go
func CreateDashboardLogger(servicesWithBuild []compose.ServiceBuildInfo) (BuildLogger, *BuildDashboard) {
    if term.IsTerminal(int(os.Stdout.Fd())) && len(servicesWithBuild) > 1 {
        serviceNames := make([]string, len(servicesWithBuild))
        for i, svc := range servicesWithBuild {
            serviceNames[i] = svc.ServiceName
        }
        dashboard := NewBuildDashboard(serviceNames)
        logger := NewDashboardBuildLogger(dashboard)
        dashboard.Start()
        return logger, dashboard
    }
    return NewStyledBuildLogger("BUILD"), nil
}

[Copilot]

Style objects are being created on every render call (lines 222-232, 269-271, 303-305, 318-320), which is inefficient. Consider defining these styles as fields in the dashboardModel struct and initializing them once in NewBuildDashboard, similar to how it's done in StyledBuildLogger (see internal/build/logger.go lines 14-35). This will improve performance during frequent redraws.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

[nitpick] Service names longer than 20 characters will cause display misalignment. Consider truncating long service names or making the width dynamic based on the longest service name:

// In NewBuildDashboard, calculate max name length:
maxNameLen := 20
for _, name := range services {
    if len(name) > maxNameLen {
        maxNameLen = len(name)
    }
}
// Store maxNameLen in the model and use it in formatting

[Copilot]

Potential race condition: The UpdateService and Stop methods both access bd.program without synchronization. If Stop() is called concurrently with UpdateService(), there's a race on the nil check and the method call. Consider adding a mutex to protect bd.program access, or use atomic operations.

Example:

type BuildDashboard struct {
    program      *tea.Program
    model        *dashboardModel
    serviceOrder []string
    mu           sync.RWMutex // Add mutex
}

func (bd *BuildDashboard) UpdateService(...) {
    bd.mu.RLock()
    defer bd.mu.RUnlock()
    if bd.program != nil {
        bd.program.Send(...)
    }
}

func (bd *BuildDashboard) Stop() {
    bd.mu.Lock()
    defer bd.mu.Unlock()
    if bd.program != nil {
        bd.program.Quit()
    }
}

[Copilot]

[nitpick] Consider extracting the magic number 3 to a named constant for better maintainability:

const maxLogLinesPerService = 3

This would make it easier to adjust this limit in the future and improves code clarity.

[Copilot]

Dashboard errors should be written to stderr instead of stdout using fmt.Fprintf(os.Stderr, ...) to avoid mixing error messages with normal program output. This is especially important in CI/CD pipelines where stdout and stderr are often processed separately.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.