diff --git a/internal/container/start.go b/internal/container/start.go index 84fe145c..0aa4ba00 100644 --- a/internal/container/start.go +++ b/internal/container/start.go @@ -174,7 +174,7 @@ func Start(ctx context.Context, rt runtime.Runtime, sink output.Sink, opts Start return "", err } - pulled, err := pullImages(ctx, rt, sink, tel, containers) + pulled, err := pullImages(ctx, rt, sink, tel, containers, interactive) if err != nil { return "", err } @@ -306,7 +306,7 @@ func tipsForType(t config.EmulatorType) []string { return nil } -func pullImages(ctx context.Context, rt runtime.Runtime, sink output.Sink, tel *telemetry.Client, containers []runtime.ContainerConfig) (map[string]bool, error) { +func pullImages(ctx context.Context, rt runtime.Runtime, sink output.Sink, tel *telemetry.Client, containers []runtime.ContainerConfig, interactive bool) (map[string]bool, error) { pulled := make(map[string]bool, len(containers)) for _, c := range containers { // Remove any existing stopped container with the same name @@ -314,48 +314,107 @@ func pullImages(ctx context.Context, rt runtime.Runtime, sink output.Sink, tel * return nil, fmt.Errorf("failed to remove existing container %s: %w", c.Name, err) } + exists, err := rt.ImageExists(ctx, c.Image) + if err != nil { + return nil, fmt.Errorf("failed to check for local image %s: %w", c.Image, err) + } + // Reuse a locally present image for pinned tags instead of re-pulling. // Floating "latest"/empty tags always pull until pull_policy support lands. - if c.Tag != "" && c.Tag != "latest" { - exists, err := rt.ImageExists(ctx, c.Image) - if err != nil { - return nil, fmt.Errorf("failed to check for local image %s: %w", c.Image, err) - } - if exists { - sink.Emit(output.MessageEvent{Severity: output.SeveritySuccess, Text: fmt.Sprintf("Using local image %s", c.Image)}) - pulled[c.Name] = false - continue - } + if exists && c.Tag != "" && c.Tag != "latest" { + sink.Emit(output.MessageEvent{Severity: output.SeveritySuccess, Text: fmt.Sprintf("Using local image %s", c.Image)}) + pulled[c.Name] = false + continue } - sink.Emit(output.SpinnerStart(fmt.Sprintf("Pulling %s", c.Image))) - sink.Emit(output.ContainerStatusEvent{Phase: "pulling", Container: c.Image}) - progress := make(chan runtime.PullProgress) - go func() { - for p := range progress { - sink.Emit(output.ProgressEvent{Container: c.Image, LayerID: p.LayerID, Status: p.Status, Current: p.Current, Total: p.Total}) + usedLocal, err := pullImage(ctx, rt, sink, tel, c, exists, interactive) + if err != nil { + return nil, err + } + pulled[c.Name] = !usedLocal + } + return pulled, nil +} + +// pullImage pulls c.Image, with a graceful fall-back to an already-present local +// image. When a local copy exists and we're interactive, the user can press ESC +// to abandon the in-flight pull and keep the current image; the same fall-back +// happens automatically when the pull fails (e.g. offline). Returns usedLocal=true +// when the pull was skipped or failed and the local image is used instead. +func pullImage(ctx context.Context, rt runtime.Runtime, sink output.Sink, tel *telemetry.Client, c runtime.ContainerConfig, localExists, interactive bool) (usedLocal bool, err error) { + sink.Emit(output.SpinnerStart(fmt.Sprintf("Pulling %s", c.Image))) + sink.Emit(output.ContainerStatusEvent{Phase: "pulling", Container: c.Image}) + + pullCtx, cancelPull := context.WithCancel(ctx) + defer cancelPull() + + // skipCh is signaled by the TUI when the user presses ESC during the pull. + // Buffered so the TUI never blocks if the pull has already finished. + skipCh := make(chan struct{}, 1) + skippable := localExists && interactive + + progress := make(chan runtime.PullProgress) + progressDone := make(chan struct{}) + go func() { + defer close(progressDone) + armed := false + for p := range progress { + // Surface the ESC hint only once real layer download begins, so it + // never flashes on an "up to date" / cache-hit pull. + if skippable && !armed && p.Status == "Downloading" { + armed = true + sink.Emit(output.PullSkippableEvent{Image: c.Image, SkipCh: skipCh}) } - }() - if err := rt.PullImage(ctx, c.Image, progress); err != nil { - sink.Emit(output.SpinnerStop()) - sink.Emit(output.ErrorEvent{ - Title: fmt.Sprintf("Failed to pull %s", c.Image), - Summary: err.Error(), - }) - tel.EmitEmulatorLifecycleEvent(ctx, telemetry.LifecycleEvent{ - EventType: telemetry.LifecycleStartError, - Emulator: c.EmulatorType, - Image: c.Image, - ErrorCode: telemetry.ErrCodeImagePullFailed, - ErrorMsg: err.Error(), - }) - return nil, output.NewSilentError(fmt.Errorf("failed to pull image %s: %w", c.Image, err)) + sink.Emit(output.ProgressEvent{Container: c.Image, LayerID: p.LayerID, Status: p.Status, Current: p.Current, Total: p.Total}) } + }() + + pullErrCh := make(chan error, 1) + go func() { pullErrCh <- rt.PullImage(pullCtx, c.Image, progress) }() + + select { + case err = <-pullErrCh: + <-progressDone + case <-skipCh: + cancelPull() + <-pullErrCh // let the pull goroutine unwind after cancellation + <-progressDone sink.Emit(output.SpinnerStop()) - sink.Emit(output.MessageEvent{Severity: output.SeveritySuccess, Text: fmt.Sprintf("Pulled %s", c.Image)}) - pulled[c.Name] = true + sink.Emit(output.MessageEvent{Severity: output.SeverityNote, Text: fmt.Sprintf("Keeping current local image %s", c.Image)}) + return true, nil } - return pulled, nil + + sink.Emit(output.SpinnerStop()) + + if err != nil { + // A cancelled parent context (e.g. Ctrl+C) is a deliberate abort, not a pull + // failure: propagate it so the start flow stops instead of silently falling + // back to the local image. (ESC-to-skip is handled by the skipCh branch above.) + if errors.Is(err, context.Canceled) { + return false, err + } + // Auto fall-back: a failed pull with a local copy present (e.g. offline) is + // not fatal — start with the image we have and tell the user. + if localExists { + sink.Emit(output.MessageEvent{Severity: output.SeverityNote, Text: fmt.Sprintf("Could not pull %s (%v); using local image", c.Image, err)}) + return true, nil + } + sink.Emit(output.ErrorEvent{ + Title: fmt.Sprintf("Failed to pull %s", c.Image), + Summary: err.Error(), + }) + tel.EmitEmulatorLifecycleEvent(ctx, telemetry.LifecycleEvent{ + EventType: telemetry.LifecycleStartError, + Emulator: c.EmulatorType, + Image: c.Image, + ErrorCode: telemetry.ErrCodeImagePullFailed, + ErrorMsg: err.Error(), + }) + return false, output.NewSilentError(fmt.Errorf("failed to pull image %s: %w", c.Image, err)) + } + + sink.Emit(output.MessageEvent{Severity: output.SeveritySuccess, Text: fmt.Sprintf("Pulled %s", c.Image)}) + return false, nil } // Validates licenses before pulling for containers with pinned tags. diff --git a/internal/container/start_test.go b/internal/container/start_test.go index 4d25bf4f..c2ca0cf2 100644 --- a/internal/container/start_test.go +++ b/internal/container/start_test.go @@ -9,6 +9,8 @@ import ( "net/http" "net/http/httptest" "strconv" + "strings" + "sync" "testing" "github.com/localstack/lstk/internal/caller" @@ -494,7 +496,7 @@ func TestPullImages_ReusesLocalImageWhenPresent(t *testing.T) { var out bytes.Buffer sink := output.NewPlainSink(&out) - pulled, err := pullImages(context.Background(), mockRT, sink, mockTel, []runtime.ContainerConfig{c}) + pulled, err := pullImages(context.Background(), mockRT, sink, mockTel, []runtime.ContainerConfig{c}, false) require.NoError(t, err) assert.False(t, pulled[c.Name], "telemetry must not count a reused image as pulled") @@ -524,9 +526,232 @@ func TestPullImages_PullsWhenImageMissing(t *testing.T) { var out bytes.Buffer sink := output.NewPlainSink(&out) - pulled, err := pullImages(context.Background(), mockRT, sink, mockTel, []runtime.ContainerConfig{c}) + pulled, err := pullImages(context.Background(), mockRT, sink, mockTel, []runtime.ContainerConfig{c}, false) require.NoError(t, err) assert.True(t, pulled[c.Name], "a freshly pulled image must be counted as pulled") assert.Contains(t, out.String(), "Pulled localstack/localstack-pro:3.5.0") } + +// recordingSink captures emitted events and optionally reacts to a +// PullSkippableEvent (mimicking the TUI binding ESC) by invoking onSkippable. +type recordingSink struct { + mu sync.Mutex + events []output.Event + onSkippable func(output.PullSkippableEvent) +} + +func (s *recordingSink) Emit(e output.Event) { + s.mu.Lock() + s.events = append(s.events, e) + cb := s.onSkippable + s.mu.Unlock() + if ev, ok := e.(output.PullSkippableEvent); ok && cb != nil { + cb(ev) + } +} + +func (s *recordingSink) messageTexts() []string { + s.mu.Lock() + defer s.mu.Unlock() + var texts []string + for _, e := range s.events { + if m, ok := e.(output.MessageEvent); ok { + texts = append(texts, m.Text) + } + } + return texts +} + +func (s *recordingSink) sawSkippable() bool { + s.mu.Lock() + defer s.mu.Unlock() + for _, e := range s.events { + if _, ok := e.(output.PullSkippableEvent); ok { + return true + } + } + return false +} + +func (s *recordingSink) sawError() bool { + s.mu.Lock() + defer s.mu.Unlock() + for _, e := range s.events { + if _, ok := e.(output.ErrorEvent); ok { + return true + } + } + return false +} + +// PRO-324: ESC during a floating-tag pull cancels the pull and falls back to the +// already-present local image without erroring out. +func TestPullImages_EscSkipsPullAndUsesLocal(t *testing.T) { + ctrl := gomock.NewController(t) + mockRT := runtime.NewMockRuntime(ctrl) + mockTel := telemetry.New("", true) + + c := runtime.ContainerConfig{ + Image: "localstack/localstack-pro:latest", + Name: "localstack-aws", + EmulatorType: config.EmulatorAWS, + Tag: "latest", + } + + mockRT.EXPECT().Remove(gomock.Any(), c.Name).Return(nil) + mockRT.EXPECT().ImageExists(gomock.Any(), c.Image).Return(true, nil) + mockRT.EXPECT().PullImage(gomock.Any(), c.Image, gomock.Any()). + DoAndReturn(func(ctx context.Context, _ string, progress chan<- runtime.PullProgress) error { + // Report real layer download so the pull becomes skippable, then block + // until the domain cancels in response to the ESC signal. + progress <- runtime.PullProgress{LayerID: "layer1", Status: "Downloading", Current: 10, Total: 100} + <-ctx.Done() + close(progress) + return ctx.Err() + }) + + sink := &recordingSink{} + sink.onSkippable = func(ev output.PullSkippableEvent) { ev.SkipCh <- struct{}{} } + + pulled, err := pullImages(context.Background(), mockRT, sink, mockTel, []runtime.ContainerConfig{c}, true) + + require.NoError(t, err) + assert.False(t, pulled[c.Name], "a skipped pull must not be counted as pulled") + assert.True(t, sink.sawSkippable(), "a skippable pull event should be emitted once download begins") + assert.False(t, sink.sawError(), "skipping the pull must not surface an error") + assert.Contains(t, strings.Join(sink.messageTexts(), "\n"), "Keeping current local image localstack/localstack-pro:latest") +} + +// PRO-324: a pull that fails with a local copy present falls back automatically +// (e.g. offline at a booth) instead of aborting the start. +func TestPullImages_PullErrorFallsBackToLocal(t *testing.T) { + ctrl := gomock.NewController(t) + mockRT := runtime.NewMockRuntime(ctrl) + mockTel := telemetry.New("", true) + + c := runtime.ContainerConfig{ + Image: "localstack/localstack-pro:latest", + Name: "localstack-aws", + EmulatorType: config.EmulatorAWS, + Tag: "latest", + } + + mockRT.EXPECT().Remove(gomock.Any(), c.Name).Return(nil) + mockRT.EXPECT().ImageExists(gomock.Any(), c.Image).Return(true, nil) + mockRT.EXPECT().PullImage(gomock.Any(), c.Image, gomock.Any()). + DoAndReturn(func(_ context.Context, _ string, progress chan<- runtime.PullProgress) error { + close(progress) + return errors.New("no route to host") + }) + + sink := &recordingSink{} + + pulled, err := pullImages(context.Background(), mockRT, sink, mockTel, []runtime.ContainerConfig{c}, false) + + require.NoError(t, err, "a failed pull with a local image present must not be fatal") + assert.False(t, pulled[c.Name], "a fall-back image must not be counted as pulled") + assert.False(t, sink.sawError(), "the fall-back must not surface an ErrorEvent") + assert.Contains(t, strings.Join(sink.messageTexts(), "\n"), "Could not pull localstack/localstack-pro:latest") +} + +// PRO-324: a failed pull with no local copy stays fatal (preserves prior behavior). +func TestPullImages_PullErrorWithoutLocalIsFatal(t *testing.T) { + ctrl := gomock.NewController(t) + mockRT := runtime.NewMockRuntime(ctrl) + mockTel := telemetry.New("", true) + + c := runtime.ContainerConfig{ + Image: "localstack/localstack-pro:latest", + Name: "localstack-aws", + EmulatorType: config.EmulatorAWS, + Tag: "latest", + } + + mockRT.EXPECT().Remove(gomock.Any(), c.Name).Return(nil) + mockRT.EXPECT().ImageExists(gomock.Any(), c.Image).Return(false, nil) + mockRT.EXPECT().PullImage(gomock.Any(), c.Image, gomock.Any()). + DoAndReturn(func(_ context.Context, _ string, progress chan<- runtime.PullProgress) error { + close(progress) + return errors.New("manifest unknown") + }) + + sink := &recordingSink{} + + _, err := pullImages(context.Background(), mockRT, sink, mockTel, []runtime.ContainerConfig{c}, true) + + require.Error(t, err) + assert.True(t, output.IsSilent(err), "error should be silent since an ErrorEvent was emitted") + assert.True(t, sink.sawError(), "a fatal pull failure must surface an ErrorEvent") +} + +// PRO-324: in non-interactive mode no skippable-pull affordance is offered, even +// when a local copy exists; the pull just proceeds. +func TestPullImages_NonInteractiveNeverSkippable(t *testing.T) { + ctrl := gomock.NewController(t) + mockRT := runtime.NewMockRuntime(ctrl) + mockTel := telemetry.New("", true) + + c := runtime.ContainerConfig{ + Image: "localstack/localstack-pro:latest", + Name: "localstack-aws", + EmulatorType: config.EmulatorAWS, + Tag: "latest", + } + + mockRT.EXPECT().Remove(gomock.Any(), c.Name).Return(nil) + mockRT.EXPECT().ImageExists(gomock.Any(), c.Image).Return(true, nil) + mockRT.EXPECT().PullImage(gomock.Any(), c.Image, gomock.Any()). + DoAndReturn(func(_ context.Context, _ string, progress chan<- runtime.PullProgress) error { + progress <- runtime.PullProgress{LayerID: "layer1", Status: "Downloading", Current: 10, Total: 100} + close(progress) + return nil + }) + + sink := &recordingSink{} + + pulled, err := pullImages(context.Background(), mockRT, sink, mockTel, []runtime.ContainerConfig{c}, false) + + require.NoError(t, err) + assert.True(t, pulled[c.Name], "a completed pull must be counted as pulled") + assert.False(t, sink.sawSkippable(), "non-interactive mode must never offer to skip the pull") +} + +// PRO-324: cancelling the parent context (e.g. Ctrl+C) during a pull is a +// deliberate abort, not a pull failure — it must propagate as context.Canceled +// rather than being swallowed by the local-image fall-back. +func TestPullImages_ParentCancelPropagatesNotFallBack(t *testing.T) { + ctrl := gomock.NewController(t) + mockRT := runtime.NewMockRuntime(ctrl) + mockTel := telemetry.New("", true) + + c := runtime.ContainerConfig{ + Image: "localstack/localstack-pro:latest", + Name: "localstack-aws", + EmulatorType: config.EmulatorAWS, + Tag: "latest", + } + + ctx, cancel := context.WithCancel(context.Background()) + + mockRT.EXPECT().Remove(gomock.Any(), c.Name).Return(nil) + mockRT.EXPECT().ImageExists(gomock.Any(), c.Image).Return(true, nil) + mockRT.EXPECT().PullImage(gomock.Any(), c.Image, gomock.Any()). + DoAndReturn(func(pullCtx context.Context, _ string, progress chan<- runtime.PullProgress) error { + // Simulate Ctrl+C: the parent ctx is cancelled mid-pull, which cancels + // the derived pullCtx; the runtime then returns its context error. + cancel() + <-pullCtx.Done() + close(progress) + return pullCtx.Err() + }) + + sink := &recordingSink{} + + _, err := pullImages(ctx, mockRT, sink, mockTel, []runtime.ContainerConfig{c}, false) + + require.Error(t, err, "a cancelled pull must not be swallowed as a successful fall-back") + assert.True(t, errors.Is(err, context.Canceled), "parent cancellation must propagate") + assert.NotContains(t, strings.Join(sink.messageTexts(), "\n"), "using local image", + "cancellation must not emit the offline fall-back message") +} diff --git a/internal/output/events.go b/internal/output/events.go index c4455f93..6823b2d3 100644 --- a/internal/output/events.go +++ b/internal/output/events.go @@ -146,6 +146,7 @@ func (SnapshotShownEvent) sealedEvent() {} func (ContainerStatusEvent) sealedEvent() {} func (ProgressEvent) sealedEvent() {} func (UserInputRequestEvent) sealedEvent() {} +func (PullSkippableEvent) sealedEvent() {} func (LogLineEvent) sealedEvent() {} type Sink interface { @@ -192,6 +193,17 @@ type UserInputRequestEvent struct { Vertical bool } +// PullSkippableEvent signals that an in-flight image pull can be abandoned in +// favor of an already-present local image. The domain emits it once real layer +// download begins (interactive mode, with a local copy present); the TUI binds +// ESC during the pull to send on SkipCh, which the domain selects on to cancel +// the pull and fall back to the local image. Never emitted in non-interactive +// mode, so PlainSink never needs to answer it. +type PullSkippableEvent struct { + Image string + SkipCh chan<- struct{} +} + const ( LogSourceEmulator = "emulator" LogSourceBrew = "brew" diff --git a/internal/output/plain_format.go b/internal/output/plain_format.go index 1aff3adb..2dcfc9ed 100644 --- a/internal/output/plain_format.go +++ b/internal/output/plain_format.go @@ -32,6 +32,10 @@ func FormatEventLine(event Event) (string, bool) { return "", false case UserInputRequestEvent: return formatUserInputRequest(e), true + case PullSkippableEvent: + // Interactive-only affordance with no plain-text rendering: non-interactive + // pulls never emit it, and PlainSink cannot bind the ESC key. + return "", false case LogLineEvent: return e.Line, true case InstanceInfoEvent: diff --git a/internal/output/plain_sink_test.go b/internal/output/plain_sink_test.go index fb167b72..379745c7 100644 --- a/internal/output/plain_sink_test.go +++ b/internal/output/plain_sink_test.go @@ -107,6 +107,15 @@ func TestPlainSink_SuppressesProgressEvent(t *testing.T) { assert.Equal(t, "", out.String()) } +func TestPlainSink_SuppressesPullSkippableEvent(t *testing.T) { + var out bytes.Buffer + sink := NewPlainSink(&out) + + sink.Emit(PullSkippableEvent{Image: "localstack/localstack-pro:latest", SkipCh: make(chan struct{}, 1)}) + + assert.Equal(t, "", out.String(), "the skippable-pull affordance has no plain-text rendering") +} + func TestPlainSink_EmitsLogLineEvent(t *testing.T) { var out bytes.Buffer sink := NewPlainSink(&out) diff --git a/internal/runtime/docker.go b/internal/runtime/docker.go index 0dad2eb5..e59ee633 100644 --- a/internal/runtime/docker.go +++ b/internal/runtime/docker.go @@ -181,6 +181,12 @@ func windowsDockerStartCommand(getenv func(string) string, lookPath func(string) } func (d *DockerRuntime) PullImage(ctx context.Context, imageName string, progress chan<- PullProgress) error { + // Close progress unconditionally — even if ImagePull fails before returning a + // reader — so callers that wait for the progress stream to drain never hang. + if progress != nil { + defer close(progress) + } + reader, err := d.client.ImagePull(ctx, imageName, client.ImagePullOptions{}) if err != nil { return err @@ -191,10 +197,6 @@ func (d *DockerRuntime) PullImage(ctx context.Context, imageName string, progres } }() - if progress != nil { - defer close(progress) - } - decoder := json.NewDecoder(reader) for { var msg struct { diff --git a/internal/runtime/docker_test.go b/internal/runtime/docker_test.go index 284aa453..ed3858e6 100644 --- a/internal/runtime/docker_test.go +++ b/internal/runtime/docker_test.go @@ -1,11 +1,13 @@ package runtime import ( + "context" "errors" "net" "os" "path/filepath" "testing" + "time" "github.com/moby/moby/client" "github.com/stretchr/testify/assert" @@ -29,6 +31,36 @@ func listenUnixSocket(t *testing.T, path string) { t.Cleanup(func() { _ = l.Close() }) } +// PRO-324 regression: PullImage must close the progress channel even when the +// underlying ImagePull call fails before it returns a reader (e.g. the daemon is +// unreachable). The caller (container.pullImage) waits for the progress stream to +// drain, so a PullImage that returns without closing progress hangs the start flow. +func TestPullImage_ClosesProgressOnImmediateError(t *testing.T) { + t.Parallel() + + cli, err := client.New(client.WithHost("unix:///tmp/lstk-nonexistent-pull.sock")) + require.NoError(t, err) + rt := &DockerRuntime{client: cli} + + progress := make(chan PullProgress) + drained := make(chan struct{}) + go func() { + defer close(drained) + for range progress { + } + }() + + pullErr := rt.PullImage(context.Background(), "localstack/localstack:latest", progress) + require.Error(t, pullErr, "a pull against an unreachable daemon must fail") + + select { + case <-drained: + // progress was closed, so the drain goroutine could exit. + case <-time.After(2 * time.Second): + t.Fatal("PullImage returned without closing the progress channel") + } +} + func TestProbeSocket_ReturnsFirstLive(t *testing.T) { dir := shortTempDir(t) sock1 := filepath.Join(dir, "first.sock") diff --git a/internal/ui/app.go b/internal/ui/app.go index 70acd13a..8375677d 100644 --- a/internal/ui/app.go +++ b/internal/ui/app.go @@ -18,6 +18,10 @@ import ( const maxLines = 200 +// pullSkipHint replaces the "Pulling " spinner text once a skippable pull +// starts downloading, telling the user they can bail out to the local image. +const pullSkipHint = "Pulling new version… (press esc to keep current version)" + type runDoneMsg struct{} type runErrMsg struct { @@ -55,6 +59,7 @@ type App struct { width int cancel func() pendingInput *output.UserInputRequestEvent + pullSkip *output.PullSkippableEvent err error quitting bool hideHeader bool @@ -133,6 +138,12 @@ func (a App) Update(msg tea.Msg) (tea.Model, tea.Cmd) { } return a, tea.Quit } + if msg.String() == "esc" && a.pullSkip != nil { + skipCmd := sendPullSkipCmd(a.pullSkip.SkipCh) + a.pullSkip = nil + a.spinner = a.spinner.SetText("") + return a, skipCmd + } if a.pendingInput != nil { if a.pendingInput.Vertical { return a.handleVerticalPromptKey(msg) @@ -179,6 +190,11 @@ func (a App) Update(msg tea.Msg) (tea.Model, tea.Cmd) { } else { a.inputPrompt = a.inputPrompt.Show(msg.Prompt, msg.Options, msg.Vertical) } + case output.PullSkippableEvent: + msgCopy := msg + a.pullSkip = &msgCopy + a.spinner = a.spinner.SetText(pullSkipHint) + return a, nil case spinner.TickMsg: var cmd tea.Cmd a.spinner, cmd = a.spinner.Update(msg) @@ -188,6 +204,7 @@ func (a App) Update(msg tea.Msg) (tea.Model, tea.Cmd) { a.spinner = a.spinner.Start(msg.Text, msg.MinDuration) return a, a.spinner.Tick() } + a.pullSkip = nil if a.pullProgress.Visible() { a.pullProgress = a.pullProgress.Hide() } @@ -338,6 +355,19 @@ func sendInputResponseCmd(responseCh chan<- output.InputResponse, response outpu } } +// sendPullSkipCmd signals the domain (over the buffered SkipCh it owns) that the +// user pressed ESC to abandon the pull. The send runs in a goroutine so Update +// never blocks even if the pull has already finished and stopped reading. +func sendPullSkipCmd(skipCh chan<- struct{}) tea.Cmd { + if skipCh == nil { + return nil + } + return func() tea.Msg { + go func() { skipCh <- struct{}{} }() + return nil + } +} + func appendLine(lines []styledLine, line styledLine) []styledLine { lines = append(lines, line) if len(lines) > maxLines { diff --git a/internal/ui/app_test.go b/internal/ui/app_test.go index e91d2631..f04b28f9 100644 --- a/internal/ui/app_test.go +++ b/internal/ui/app_test.go @@ -101,6 +101,67 @@ func TestAppQuitCancelsContext(t *testing.T) { } } +// PRO-324: ESC during a skippable pull signals the domain's skip channel and +// shows the bail-out hint, without quitting the program. +func TestAppEscSkipsPullWithoutQuitting(t *testing.T) { + t.Parallel() + + cancelled := false + skipCh := make(chan struct{}, 1) + app := NewApp("dev", "", "", func() { cancelled = true }) + + model, _ := app.Update(output.SpinnerEvent{Active: true, Text: "Pulling img"}) + app = model.(App) + model, _ = app.Update(output.PullSkippableEvent{Image: "img", SkipCh: skipCh}) + app = model.(App) + if app.pullSkip == nil { + t.Fatal("expected the pull to be marked skippable") + } + if !strings.Contains(app.View(), "press esc to keep current version") { + t.Fatal("expected the ESC hint to be shown once the pull is skippable") + } + + model, cmd := app.Update(tea.KeyMsg{Type: tea.KeyEsc}) + app = model.(App) + if app.pullSkip != nil { + t.Fatal("expected pullSkip to be cleared after ESC") + } + if cancelled { + t.Fatal("ESC must not cancel the program context") + } + if cmd == nil { + t.Fatal("expected a command to signal the skip channel") + } + if msg := cmd(); msg != nil { + if _, isQuit := msg.(tea.QuitMsg); isQuit { + t.Fatal("ESC must not quit the program") + } + } + + select { + case <-skipCh: + case <-time.After(time.Second): + t.Fatal("expected a signal on the skip channel after ESC") + } +} + +// PRO-324: ESC is inert unless a skippable pull is in flight. +func TestAppEscWithoutSkippablePullIsInert(t *testing.T) { + t.Parallel() + + cancelled := false + app := NewApp("dev", "", "", func() { cancelled = true }) + _, cmd := app.Update(tea.KeyMsg{Type: tea.KeyEsc}) + if cancelled { + t.Fatal("ESC with no skippable pull must not cancel the context") + } + if cmd != nil { + if _, isQuit := cmd().(tea.QuitMsg); isQuit { + t.Fatal("ESC with no skippable pull must not quit") + } + } +} + func TestAppEnterRespondsToInputRequest(t *testing.T) { t.Parallel()