fix(preview): address CodeRabbit review findings
- Add DefaultPreviewSeparator constant, use in CLI route filtering - Add PreviewStatusTimeout constant, replace raw "timeout" string - Log s.Update errors instead of silently discarding - Use cliWritef for non-interactive spinner fallback output - Use domain.ErrRouteNotFound/ErrPreviewNotFound sentinels in tests and remote client - Distinguish route-not-found from real errors in TTL cleanup
- commit
- fb3fd37bb72569e7c7e592e14a31ac568670b092
- author
- brice · · 2026-03-18 22:21 UTC
- committer
- brice · · 2026-03-18 22:04 UTC
- parent
- 459b87f
- signature
- unsigned
8 files · +29 -13
M internal/adapters/in/cli/preview.go +7 -5
| @@ -255,7 +255,7 @@ func resolveRegistryForImage(ctx context.Context, cp ControlPlane, imageName str | |||
| 255 | 255 | } | |
| 256 | 256 | ||
| 257 | 257 | for _, r := range routes { | |
| 258 | - | if strings.Contains(r.Domain, "--") { | |
| 258 | + | if strings.Contains(r.Domain, domain.DefaultPreviewSeparator) { | |
| 259 | 259 | continue | |
| 260 | 260 | } | |
| 261 | 261 | reg, _, _ := parseImageRef(r.Image) | |
| @@ -320,7 +320,7 @@ func waitForPreview(ctx context.Context, out io.Writer, name, ttl string, noData | |||
| 320 | 320 | return cliWriteLine(out, cliRenderSuccess(fmt.Sprintf("Push complete. Preview %q will be created by the server (use --remote to poll status).", name))) | |
| 321 | 321 | } | |
| 322 | 322 | ||
| 323 | - | result, err := pollPreviewWithSpinner(ctx, client, name) | |
| 323 | + | result, err := pollPreviewWithSpinner(ctx, out, client, name) | |
| 324 | 324 | if err != nil { | |
| 325 | 325 | return err | |
| 326 | 326 | } | |
| @@ -396,7 +396,7 @@ func waitForPreviewPollDone(done <-chan previewPollResult) tea.Cmd { | |||
| 396 | 396 | } | |
| 397 | 397 | } | |
| 398 | 398 | ||
| 399 | - | func pollPreviewWithSpinner(ctx context.Context, client *remote.Client, name string) (*domain.PreviewRoute, error) { | |
| 399 | + | func pollPreviewWithSpinner(ctx context.Context, out io.Writer, client *remote.Client, name string) (*domain.PreviewRoute, error) { | |
| 400 | 400 | done := make(chan previewPollResult, 1) | |
| 401 | 401 | go func() { | |
| 402 | 402 | result, err := pollPreviewStatus(ctx, client, name) | |
| @@ -404,7 +404,9 @@ func pollPreviewWithSpinner(ctx context.Context, client *remote.Client, name str | |||
| 404 | 404 | }() | |
| 405 | 405 | ||
| 406 | 406 | if !isInteractiveTerminal() { | |
| 407 | - | fmt.Printf("Waiting for preview %s...\n", name) | |
| 407 | + | if err := cliWritef(out, "Waiting for preview %s...\n", name); err != nil { | |
| 408 | + | return nil, err | |
| 409 | + | } | |
| 408 | 410 | r := <-done | |
| 409 | 411 | return r.preview, r.err | |
| 410 | 412 | } | |
| @@ -433,7 +435,7 @@ func pollPreviewStatus(ctx context.Context, client *remote.Client, name string) | |||
| 433 | 435 | case <-ctx.Done(): | |
| 434 | 436 | return nil, ctx.Err() | |
| 435 | 437 | case <-timeout: | |
| 436 | - | return &domain.PreviewRoute{Status: "timeout"}, nil | |
| 438 | + | return &domain.PreviewRoute{Status: domain.PreviewStatusTimeout}, nil | |
| 437 | 439 | case <-ticker.C: | |
| 438 | 440 | p, err := client.GetPreview(ctx, name) | |
| 439 | 441 | if err != nil { | |
| @@ -255,7 +255,7 @@ func resolveRegistryForImage(ctx context.Context, cp ControlPlane, imageName str | |||||
| 255 | } | 255 | } | ||
| 256 | 256 | ||||
| 257 | for _, r := range routes { | 257 | for _, r := range routes { | ||
| 258 | - | if strings.Contains(r.Domain, "--") { | 258 | + | if strings.Contains(r.Domain, domain.DefaultPreviewSeparator) { |
| 259 | continue | 259 | continue | ||
| 260 | } | 260 | } | ||
| 261 | reg, _, _ := parseImageRef(r.Image) | 261 | reg, _, _ := parseImageRef(r.Image) | ||
| @@ -320,7 +320,7 @@ func waitForPreview(ctx context.Context, out io.Writer, name, ttl string, noData | |||||
| 320 | return cliWriteLine(out, cliRenderSuccess(fmt.Sprintf("Push complete. Preview %q will be created by the server (use --remote to poll status).", name))) | 320 | return cliWriteLine(out, cliRenderSuccess(fmt.Sprintf("Push complete. Preview %q will be created by the server (use --remote to poll status).", name))) | ||
| 321 | } | 321 | } | ||
| 322 | 322 | ||||
| 323 | - | result, err := pollPreviewWithSpinner(ctx, client, name) | 323 | + | result, err := pollPreviewWithSpinner(ctx, out, client, name) |
| 324 | if err != nil { | 324 | if err != nil { | ||
| 325 | return err | 325 | return err | ||
| 326 | } | 326 | } | ||
| @@ -396,7 +396,7 @@ func waitForPreviewPollDone(done <-chan previewPollResult) tea.Cmd { | |||||
| 396 | } | 396 | } | ||
| 397 | } | 397 | } | ||
| 398 | 398 | ||||
| 399 | - | func pollPreviewWithSpinner(ctx context.Context, client *remote.Client, name string) (*domain.PreviewRoute, error) { | 399 | + | func pollPreviewWithSpinner(ctx context.Context, out io.Writer, client *remote.Client, name string) (*domain.PreviewRoute, error) { |
| 400 | done := make(chan previewPollResult, 1) | 400 | done := make(chan previewPollResult, 1) | ||
| 401 | go func() { | 401 | go func() { | ||
| 402 | result, err := pollPreviewStatus(ctx, client, name) | 402 | result, err := pollPreviewStatus(ctx, client, name) | ||
| @@ -404,7 +404,9 @@ func pollPreviewWithSpinner(ctx context.Context, client *remote.Client, name str | |||||
| 404 | }() | 404 | }() | ||
| 405 | 405 | ||||
| 406 | if !isInteractiveTerminal() { | 406 | if !isInteractiveTerminal() { | ||
| 407 | - | fmt.Printf("Waiting for preview %s...\n", name) | 407 | + | if err := cliWritef(out, "Waiting for preview %s...\n", name); err != nil { |
| 408 | + | return nil, err | |||
| 409 | + | } | |||
| 408 | r := <-done | 410 | r := <-done | ||
| 409 | return r.preview, r.err | 411 | return r.preview, r.err | ||
| 410 | } | 412 | } | ||
| @@ -433,7 +435,7 @@ func pollPreviewStatus(ctx context.Context, client *remote.Client, name string) | |||||
| 433 | case <-ctx.Done(): | 435 | case <-ctx.Done(): | ||
| 434 | return nil, ctx.Err() | 436 | return nil, ctx.Err() | ||
| 435 | case <-timeout: | 437 | case <-timeout: | ||
| 436 | - | return &domain.PreviewRoute{Status: "timeout"}, nil | 438 | + | return &domain.PreviewRoute{Status: domain.PreviewStatusTimeout}, nil |
| 437 | case <-ticker.C: | 439 | case <-ticker.C: | ||
| 438 | p, err := client.GetPreview(ctx, name) | 440 | p, err := client.GetPreview(ctx, name) | ||
| 439 | if err != nil { | 441 | if err != nil { | ||
M internal/adapters/in/cli/push.go +1 -1
| @@ -280,7 +280,7 @@ func resolveFromImage(ctx context.Context, cp ControlPlane, imageArg, dockerfile | |||
| 280 | 280 | // pollute the selection when pushing the base image. | |
| 281 | 281 | filtered := routes[:0] | |
| 282 | 282 | for _, r := range routes { | |
| 283 | - | if !strings.Contains(r.Domain, "--") { | |
| 283 | + | if !strings.Contains(r.Domain, domain.DefaultPreviewSeparator) { | |
| 284 | 284 | filtered = append(filtered, r) | |
| 285 | 285 | } | |
| 286 | 286 | } | |
| @@ -280,7 +280,7 @@ func resolveFromImage(ctx context.Context, cp ControlPlane, imageArg, dockerfile | |||||
| 280 | // pollute the selection when pushing the base image. | 280 | // pollute the selection when pushing the base image. | ||
| 281 | filtered := routes[:0] | 281 | filtered := routes[:0] | ||
| 282 | for _, r := range routes { | 282 | for _, r := range routes { | ||
| 283 | - | if !strings.Contains(r.Domain, "--") { | 283 | + | if !strings.Contains(r.Domain, domain.DefaultPreviewSeparator) { |
| 284 | filtered = append(filtered, r) | 284 | filtered = append(filtered, r) | ||
| 285 | } | 285 | } | ||
| 286 | } | 286 | } | ||
M internal/adapters/in/cli/remote/preview.go +2 -2
| @@ -59,12 +59,12 @@ func (c *Client) ExtendPreview(ctx context.Context, name string, ttl string) err | |||
| 59 | 59 | func (c *Client) GetPreview(ctx context.Context, name string) (*domain.PreviewRoute, error) { | |
| 60 | 60 | previews, err := c.ListPreviews(ctx) | |
| 61 | 61 | if err != nil { | |
| 62 | - | return nil, err | |
| 62 | + | return nil, fmt.Errorf("list previews: %w", err) | |
| 63 | 63 | } | |
| 64 | 64 | for _, p := range previews { | |
| 65 | 65 | if p.Name == name { | |
| 66 | 66 | return &p, nil | |
| 67 | 67 | } | |
| 68 | 68 | } | |
| 69 | - | return nil, fmt.Errorf("preview %q not found", name) | |
| 69 | + | return nil, fmt.Errorf("preview %q: %w", name, domain.ErrPreviewNotFound) | |
| 70 | 70 | } | |
| @@ -59,12 +59,12 @@ func (c *Client) ExtendPreview(ctx context.Context, name string, ttl string) err | |||||
| 59 | func (c *Client) GetPreview(ctx context.Context, name string) (*domain.PreviewRoute, error) { | 59 | func (c *Client) GetPreview(ctx context.Context, name string) (*domain.PreviewRoute, error) { | ||
| 60 | previews, err := c.ListPreviews(ctx) | 60 | previews, err := c.ListPreviews(ctx) | ||
| 61 | if err != nil { | 61 | if err != nil { | ||
| 62 | - | return nil, err | 62 | + | return nil, fmt.Errorf("list previews: %w", err) |
| 63 | } | 63 | } | ||
| 64 | for _, p := range previews { | 64 | for _, p := range previews { | ||
| 65 | if p.Name == name { | 65 | if p.Name == name { | ||
| 66 | return &p, nil | 66 | return &p, nil | ||
| 67 | } | 67 | } | ||
| 68 | } | 68 | } | ||
| 69 | - | return nil, fmt.Errorf("preview %q not found", name) | 69 | + | return nil, fmt.Errorf("preview %q: %w", name, domain.ErrPreviewNotFound) |
| 70 | } | 70 | } | ||
M internal/app/run.go +5 -1
| @@ -599,7 +599,11 @@ func initPreviewService(ctx context.Context, cfg Config, svc *services, log zero | |||
| 599 | 599 | } | |
| 600 | 600 | // Remove route from config so proxy stops routing to this domain | |
| 601 | 601 | if err := svc.configSvc.RemoveRoute(ctx, p.Domain); err != nil { | |
| 602 | - | log.Debug().Err(err).Str("domain", p.Domain).Msg("preview route already removed from config") | |
| 602 | + | if errors.Is(err, domain.ErrRouteNotFound) { | |
| 603 | + | log.Debug().Str("domain", p.Domain).Msg("preview route already removed from config") | |
| 604 | + | } else { | |
| 605 | + | log.Warn().Err(err).Str("domain", p.Domain).Msg("failed to remove preview route from config") | |
| 606 | + | } | |
| 603 | 607 | } | |
| 604 | 608 | svc.proxySvc.InvalidateTarget(ctx, p.Domain) | |
| 605 | 609 | }) | |
| @@ -599,7 +599,11 @@ func initPreviewService(ctx context.Context, cfg Config, svc *services, log zero | |||||
| 599 | } | 599 | } | ||
| 600 | // Remove route from config so proxy stops routing to this domain | 600 | // Remove route from config so proxy stops routing to this domain | ||
| 601 | if err := svc.configSvc.RemoveRoute(ctx, p.Domain); err != nil { | 601 | if err := svc.configSvc.RemoveRoute(ctx, p.Domain); err != nil { | ||
| 602 | - | log.Debug().Err(err).Str("domain", p.Domain).Msg("preview route already removed from config") | 602 | + | if errors.Is(err, domain.ErrRouteNotFound) { |
| 603 | + | log.Debug().Str("domain", p.Domain).Msg("preview route already removed from config") | |||
| 604 | + | } else { | |||
| 605 | + | log.Warn().Err(err).Str("domain", p.Domain).Msg("failed to remove preview route from config") | |||
| 606 | + | } | |||
| 603 | } | 607 | } | ||
| 604 | svc.proxySvc.InvalidateTarget(ctx, p.Domain) | 608 | svc.proxySvc.InvalidateTarget(ctx, p.Domain) | ||
| 605 | }) | 609 | }) | ||
M internal/domain/preview.go +5 -0
| @@ -9,6 +9,11 @@ const ( | |||
| 9 | 9 | PreviewStatusDeploying PreviewStatus = "deploying" | |
| 10 | 10 | PreviewStatusRunning PreviewStatus = "running" | |
| 11 | 11 | PreviewStatusFailed PreviewStatus = "failed" | |
| 12 | + | PreviewStatusTimeout PreviewStatus = "timeout" | |
| 13 | + | ||
| 14 | + | // DefaultPreviewSeparator is the subdomain separator used to construct | |
| 15 | + | // preview domain names (e.g. "myapp--feat.example.com"). | |
| 16 | + | DefaultPreviewSeparator = "--" | |
| 12 | 17 | ) | |
| 13 | 18 | ||
| 14 | 19 | // PreviewRoute represents an ephemeral preview environment. | |
| @@ -9,6 +9,11 @@ const ( | |||||
| 9 | PreviewStatusDeploying PreviewStatus = "deploying" | 9 | PreviewStatusDeploying PreviewStatus = "deploying" | ||
| 10 | PreviewStatusRunning PreviewStatus = "running" | 10 | PreviewStatusRunning PreviewStatus = "running" | ||
| 11 | PreviewStatusFailed PreviewStatus = "failed" | 11 | PreviewStatusFailed PreviewStatus = "failed" | ||
| 12 | + | PreviewStatusTimeout PreviewStatus = "timeout" | |||
| 13 | + | ||||
| 14 | + | // DefaultPreviewSeparator is the subdomain separator used to construct | |||
| 15 | + | // preview domain names (e.g. "myapp--feat.example.com"). | |||
| 16 | + | DefaultPreviewSeparator = "--" | |||
| 12 | ) | 17 | ) | ||
| 13 | 18 | ||||
| 14 | // PreviewRoute represents an ephemeral preview environment. | 19 | // PreviewRoute represents an ephemeral preview environment. | ||
M internal/usecase/auto/preview/integration_test.go +1 -1
| @@ -44,7 +44,7 @@ func (f *fakeRouteManager) GetRoute(ctx context.Context, d string) (*domain.Rout | |||
| 44 | 44 | if f.getRouteFn != nil { | |
| 45 | 45 | return f.getRouteFn(ctx, d) | |
| 46 | 46 | } | |
| 47 | - | return nil, fmt.Errorf("not found") | |
| 47 | + | return nil, fmt.Errorf("route %q: %w", d, domain.ErrRouteNotFound) | |
| 48 | 48 | } | |
| 49 | 49 | ||
| 50 | 50 | func (f *fakeRouteManager) GetVolumeConfig() (bool, string, bool) { | |
| @@ -44,7 +44,7 @@ func (f *fakeRouteManager) GetRoute(ctx context.Context, d string) (*domain.Rout | |||||
| 44 | if f.getRouteFn != nil { | 44 | if f.getRouteFn != nil { | ||
| 45 | return f.getRouteFn(ctx, d) | 45 | return f.getRouteFn(ctx, d) | ||
| 46 | } | 46 | } | ||
| 47 | - | return nil, fmt.Errorf("not found") | 47 | + | return nil, fmt.Errorf("route %q: %w", d, domain.ErrRouteNotFound) |
| 48 | } | 48 | } | ||
| 49 | 49 | ||||
| 50 | func (f *fakeRouteManager) GetVolumeConfig() (bool, string, bool) { | 50 | func (f *fakeRouteManager) GetVolumeConfig() (bool, string, bool) { | ||
M internal/usecase/auto/preview/service.go +6 -2
| @@ -382,7 +382,9 @@ func (s *Service) CreatePreview(ctx context.Context, req CreatePreviewRequest) e | |||
| 382 | 382 | HTTPS: req.HTTPS, | |
| 383 | 383 | }); err != nil { | |
| 384 | 384 | preview.Status = domain.PreviewStatusFailed | |
| 385 | - | _ = s.Update(ctx, preview) | |
| 385 | + | if updateErr := s.Update(ctx, preview); updateErr != nil { | |
| 386 | + | log.Warn().Err(updateErr).Str("preview", req.Name).Msg("failed to update preview status") | |
| 387 | + | } | |
| 386 | 388 | return fmt.Errorf("register preview route %q: %w", req.Domain, err) | |
| 387 | 389 | } | |
| 388 | 390 | } | |
| @@ -407,7 +409,9 @@ func (s *Service) CreatePreview(ctx context.Context, req CreatePreviewRequest) e | |||
| 407 | 409 | }) | |
| 408 | 410 | if err != nil { | |
| 409 | 411 | preview.Status = domain.PreviewStatusFailed | |
| 410 | - | _ = s.Update(ctx, preview) | |
| 412 | + | if updateErr := s.Update(ctx, preview); updateErr != nil { | |
| 413 | + | log.Warn().Err(updateErr).Str("preview", req.Name).Msg("failed to update preview status") | |
| 414 | + | } | |
| 411 | 415 | return fmt.Errorf("deploy preview %q: %w", req.Name, err) | |
| 412 | 416 | } | |
| 413 | 417 | preview.Containers = []string{container.Name} | |
| @@ -382,7 +382,9 @@ func (s *Service) CreatePreview(ctx context.Context, req CreatePreviewRequest) e | |||||
| 382 | HTTPS: req.HTTPS, | 382 | HTTPS: req.HTTPS, | ||
| 383 | }); err != nil { | 383 | }); err != nil { | ||
| 384 | preview.Status = domain.PreviewStatusFailed | 384 | preview.Status = domain.PreviewStatusFailed | ||
| 385 | - | _ = s.Update(ctx, preview) | 385 | + | if updateErr := s.Update(ctx, preview); updateErr != nil { |
| 386 | + | log.Warn().Err(updateErr).Str("preview", req.Name).Msg("failed to update preview status") | |||
| 387 | + | } | |||
| 386 | return fmt.Errorf("register preview route %q: %w", req.Domain, err) | 388 | return fmt.Errorf("register preview route %q: %w", req.Domain, err) | ||
| 387 | } | 389 | } | ||
| 388 | } | 390 | } | ||
| @@ -407,7 +409,9 @@ func (s *Service) CreatePreview(ctx context.Context, req CreatePreviewRequest) e | |||||
| 407 | }) | 409 | }) | ||
| 408 | if err != nil { | 410 | if err != nil { | ||
| 409 | preview.Status = domain.PreviewStatusFailed | 411 | preview.Status = domain.PreviewStatusFailed | ||
| 410 | - | _ = s.Update(ctx, preview) | 412 | + | if updateErr := s.Update(ctx, preview); updateErr != nil { |
| 413 | + | log.Warn().Err(updateErr).Str("preview", req.Name).Msg("failed to update preview status") | |||
| 414 | + | } | |||
| 411 | return fmt.Errorf("deploy preview %q: %w", req.Name, err) | 415 | return fmt.Errorf("deploy preview %q: %w", req.Name, err) | ||
| 412 | } | 416 | } | ||
| 413 | preview.Containers = []string{container.Name} | 417 | preview.Containers = []string{container.Name} | ||
M internal/usecase/auto/preview/service_test.go +2 -1
| @@ -89,7 +89,8 @@ func TestPreviewService_Extend(t *testing.T) { | |||
| 89 | 89 | err := svc.Extend(t.Context(), "feat", 24*time.Hour) | |
| 90 | 90 | require.NoError(t, err) | |
| 91 | 91 | ||
| 92 | - | p, _ := svc.Get(t.Context(), "feat") | |
| 92 | + | p, err := svc.Get(t.Context(), "feat") | |
| 93 | + | require.NoError(t, err) | |
| 93 | 94 | assert.True(t, p.ExpiresAt.After(now.Add(23*time.Hour))) | |
| 94 | 95 | } | |
| 95 | 96 | ||
| @@ -89,7 +89,8 @@ func TestPreviewService_Extend(t *testing.T) { | |||||
| 89 | err := svc.Extend(t.Context(), "feat", 24*time.Hour) | 89 | err := svc.Extend(t.Context(), "feat", 24*time.Hour) | ||
| 90 | require.NoError(t, err) | 90 | require.NoError(t, err) | ||
| 91 | 91 | ||||
| 92 | - | p, _ := svc.Get(t.Context(), "feat") | 92 | + | p, err := svc.Get(t.Context(), "feat") |
| 93 | + | require.NoError(t, err) | |||
| 93 | assert.True(t, p.ExpiresAt.After(now.Add(23*time.Hour))) | 94 | assert.True(t, p.ExpiresAt.After(now.Add(23*time.Hour))) | ||
| 94 | } | 95 | } | ||
| 95 | 96 | ||||