From 30da88818494cca68316f75fbdc1597a6c10e990 Mon Sep 17 00:00:00 2001 From: Rob Watson Date: Tue, 8 Apr 2025 14:55:51 +0200 Subject: [PATCH] feat(ui): improve error handling on startup --- internal/app/app.go | 21 ++++++- internal/app/integration_helpers_test.go | 3 + internal/app/integration_test.go | 80 +++++++++++++++++++++--- internal/terminal/terminal.go | 42 +++++++++---- 4 files changed, 126 insertions(+), 20 deletions(-) diff --git a/internal/app/app.go b/internal/app/app.go index 8b03ac7..3dc26db 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -55,8 +55,23 @@ func Run(ctx context.Context, params RunParams) error { } defer ui.Close() + // emptyUI is a dummy function that sets the UI state to an empty state, and + // re-renders the screen. + // + // This is a workaround for a weird interaction between tview and + // tcell.SimulationScreen which leads to newly-added pages not rendering if + // the UI is not re-rendered for a second time. + // It is only needed for integration tests when rendering modals before the + // main loop starts. It would be nice to remove this but the risk/impact on + // non-test code is pretty low. + emptyUI := func() { ui.SetState(domain.AppState{}) } + containerClient, err := container.NewClient(ctx, params.DockerClient, logger.With("component", "container_client")) if err != nil { + err = fmt.Errorf("create container client: %w", err) + ui.ShowFatalErrorModal(err) + emptyUI() + <-ui.C() return err } defer containerClient.Close() @@ -70,7 +85,11 @@ func Run(ctx context.Context, params RunParams) error { Logger: logger.With("component", "mediaserver"), }) if err != nil { - return fmt.Errorf("create mediaserver: %w", err) + err = fmt.Errorf("create mediaserver: %w", err) + ui.ShowFatalErrorModal(err) + emptyUI() + <-ui.C() + return err } defer srv.Close() diff --git a/internal/app/integration_helpers_test.go b/internal/app/integration_helpers_test.go index 1a5e96a..6098f92 100644 --- a/internal/app/integration_helpers_test.go +++ b/internal/app/integration_helpers_test.go @@ -44,6 +44,9 @@ func setupSimulationScreen(t *testing.T) (tcell.SimulationScreen, chan<- termina if y > len(lines)-1 { lines = append(lines, "") } + if len(screenCells[n].Runes) == 0 { // shouldn't really happen unless there is no output + continue + } lines[y] += string(screenCells[n].Runes[0]) } diff --git a/internal/app/integration_test.go b/internal/app/integration_test.go index 6d3a9bf..5d449f7 100644 --- a/internal/app/integration_test.go +++ b/internal/app/integration_test.go @@ -4,6 +4,7 @@ package app_test import ( "context" + "errors" "fmt" "log/slog" "net" @@ -15,13 +16,16 @@ import ( "git.netflux.io/rob/octoplex/internal/app" "git.netflux.io/rob/octoplex/internal/config" "git.netflux.io/rob/octoplex/internal/container" + "git.netflux.io/rob/octoplex/internal/container/mocks" "git.netflux.io/rob/octoplex/internal/domain" "git.netflux.io/rob/octoplex/internal/terminal" "git.netflux.io/rob/octoplex/internal/testhelpers" + "github.com/docker/docker/api/types/network" dockerclient "github.com/docker/docker/client" "github.com/docker/docker/errdefs" "github.com/gdamore/tcell/v2" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" "github.com/testcontainers/testcontainers-go" "github.com/testcontainers/testcontainers-go/wait" @@ -68,6 +72,10 @@ func TestIntegration(t *testing.T) { done := make(chan struct{}) go func() { + defer func() { + done <- struct{}{} + }() + err := app.Run(ctx, app.RunParams{ ConfigService: configService, DockerClient: dockerClient, @@ -82,8 +90,6 @@ func TestIntegration(t *testing.T) { Logger: logger, }) require.NoError(t, err) - - done <- struct{}{} }() require.EventuallyWithT( @@ -258,6 +264,10 @@ func TestIntegrationDestinationValidations(t *testing.T) { done := make(chan struct{}) go func() { + defer func() { + done <- struct{}{} + }() + err := app.Run(ctx, app.RunParams{ ConfigService: configService, DockerClient: dockerClient, @@ -272,8 +282,6 @@ func TestIntegrationDestinationValidations(t *testing.T) { Logger: logger, }) require.NoError(t, err) - - done <- struct{}{} }() require.EventuallyWithT( @@ -412,6 +420,10 @@ func TestIntegrationStartupCheck(t *testing.T) { done := make(chan struct{}) go func() { + defer func() { + done <- struct{}{} + }() + err := app.Run(ctx, app.RunParams{ ConfigService: configService, DockerClient: dockerClient, @@ -426,8 +438,6 @@ func TestIntegrationStartupCheck(t *testing.T) { Logger: logger, }) require.NoError(t, err) - - done <- struct{}{} }() require.EventuallyWithT( @@ -492,6 +502,10 @@ func TestIntegrationMediaServerError(t *testing.T) { done := make(chan struct{}) go func() { + defer func() { + done <- struct{}{} + }() + err := app.Run(ctx, app.RunParams{ ConfigService: configService, DockerClient: dockerClient, @@ -506,8 +520,6 @@ func TestIntegrationMediaServerError(t *testing.T) { Logger: logger, }) require.NoError(t, err) - - done <- struct{}{} }() require.EventuallyWithT( @@ -527,3 +539,55 @@ func TestIntegrationMediaServerError(t *testing.T) { <-done } + +func TestIntegrationDockerClientError(t *testing.T) { + ctx, cancel := context.WithTimeout(t.Context(), 10*time.Minute) + defer cancel() + + logger := testhelpers.NewTestLogger(t).With("component", "integration") + + var dockerClient mocks.DockerClient + dockerClient.EXPECT().NetworkCreate(mock.Anything, mock.Anything, mock.Anything).Return(network.CreateResponse{}, errors.New("boom")) + + configService := setupConfigService(t, config.Config{Sources: config.Sources{RTMP: config.RTMPSource{Enabled: true}}}) + screen, screenCaptureC, getContents := setupSimulationScreen(t) + + done := make(chan struct{}) + go func() { + defer func() { + done <- struct{}{} + }() + + err := app.Run(ctx, app.RunParams{ + ConfigService: configService, + DockerClient: &dockerClient, + Screen: &terminal.Screen{ + Screen: screen, + Width: 200, + Height: 25, + CaptureC: screenCaptureC, + }, + ClipboardAvailable: false, + BuildInfo: domain.BuildInfo{Version: "0.0.1", GoVersion: "go1.16.3"}, + Logger: logger, + }) + require.EqualError(t, err, "create container client: network create: boom") + }() + + require.EventuallyWithT( + t, + func(t *assert.CollectT) { + assert.True(t, contentsIncludes(getContents(), "An error occurred:"), "expected to see error message") + assert.True(t, contentsIncludes(getContents(), "create container client: network create: boom"), "expected to see message") + }, + 5*time.Second, + time.Second, + "expected to see fatal error modal", + ) + printScreen(getContents, "Ater displaying the fatal error modal") + + // Quit the app, this should cause the done channel to receive. + sendKey(screen, tcell.KeyEnter, ' ') + + <-done +} diff --git a/internal/terminal/terminal.go b/internal/terminal/terminal.go index 5856c72..c93b5ac 100644 --- a/internal/terminal/terminal.go +++ b/internal/terminal/terminal.go @@ -40,7 +40,7 @@ const ( // UI is responsible for managing the terminal user interface. type UI struct { - commandCh chan Command + commandC chan Command clipboardAvailable bool configFilePath string buildInfo domain.BuildInfo @@ -216,7 +216,7 @@ func StartUI(ctx context.Context, params StartParams) (*UI, error) { app.EnableMouse(false) ui := &UI{ - commandCh: commandCh, + commandC: commandCh, clipboardAvailable: params.ClipboardAvailable, configFilePath: params.ConfigFilePath, buildInfo: params.BuildInfo, @@ -254,11 +254,11 @@ func StartUI(ctx context.Context, params StartParams) (*UI, error) { // C returns a channel that receives commands from the user interface. func (ui *UI) C() <-chan Command { - return ui.commandCh + return ui.commandC } func (ui *UI) run(ctx context.Context) { - defer close(ui.commandCh) + defer close(ui.commandC) uiDone := make(chan struct{}) go func() { @@ -379,6 +379,24 @@ func (ui *UI) ShowDestinationErrorModal(name string, err error) { <-done } +// ShowFatalErrorModal displays the provided error. It sends a CommandQuit to the +// command channel when the user selects the Quit button. +func (ui *UI) ShowFatalErrorModal(err error) { + ui.app.QueueUpdateDraw(func() { + ui.showModal( + pageNameModalFatalError, + fmt.Sprintf( + "An error occurred:\n\n%s", + err, + ), + []string{"Quit"}, + func(int, string) { + ui.commandC <- CommandQuit{} + }, + ) + }) +} + // captureScreen captures the screen and sends it to the screenCaptureC // channel, which must have been set in StartParams. // @@ -387,7 +405,8 @@ func (ui *UI) ShowDestinationErrorModal(name string, err error) { func (ui *UI) captureScreen(screen tcell.Screen) { simScreen, ok := screen.(tcell.SimulationScreen) if !ok { - ui.logger.Error("simulation screen not available") + ui.logger.Warn("captureScreen: simulation screen not available") + return } cells, w, h := simScreen.GetContents() @@ -491,6 +510,7 @@ const ( pageNameModalAbout = "modal-about" pageNameModalClipboard = "modal-clipboard" pageNameModalDestinationError = "modal-destination-error" + pageNameModalFatalError = "modal-fatal-error" pageNameModalPullProgress = "modal-pull-progress" pageNameModalQuit = "modal-quit" pageNameModalRemoveDestination = "modal-remove-destination" @@ -567,7 +587,7 @@ func (ui *UI) handleMediaServerClosed(exitReason string) { SetBackgroundColor(tcell.ColorBlack). SetTextColor(tcell.ColorWhite). SetDoneFunc(func(int, string) { - ui.commandCh <- CommandQuit{} + ui.commandC <- CommandQuit{} }) modal.SetBorderStyle(tcell.StyleDefault.Background(tcell.ColorBlack).Foreground(tcell.ColorWhite)) @@ -758,7 +778,7 @@ func (ui *UI) addDestination() { AddInputField(inputLabelName, "My stream", inputLen, nil, nil). AddInputField(inputLabelURL, "rtmp://", inputLen, nil, nil). AddButton("Add", func() { - ui.commandCh <- CommandAddDestination{ + ui.commandC <- CommandAddDestination{ DestinationName: form.GetFormItemByLabel(inputLabelName).(*tview.InputField).GetText(), URL: form.GetFormItemByLabel(inputLabelURL).(*tview.InputField).GetText(), } @@ -809,7 +829,7 @@ func (ui *UI) removeDestination() { []string{"Remove", "Cancel"}, func(buttonIndex int, _ string) { if buttonIndex == 0 { - ui.commandCh <- CommandRemoveDestination{URL: url} + ui.commandC <- CommandRemoveDestination{URL: url} } }, ) @@ -867,12 +887,12 @@ func (ui *UI) toggleDestination() { switch ss { case startStateNotStarted: ui.urlsToStartState[url] = startStateStarting - ui.commandCh <- CommandStartDestination{URL: url} + ui.commandC <- CommandStartDestination{URL: url} case startStateStarting: // do nothing return case startStateStarted: - ui.commandCh <- CommandStopDestination{URL: url} + ui.commandC <- CommandStopDestination{URL: url} } } @@ -923,7 +943,7 @@ func (ui *UI) confirmQuit() { []string{"Quit", "Cancel"}, func(buttonIndex int, _ string) { if buttonIndex == 0 { - ui.commandCh <- CommandQuit{} + ui.commandC <- CommandQuit{} } }, )