fix: Addressing review feedback

Author: yhakbar

internal/cli/commands/catalog/catalog_redesign.go

@@ -2,6 +2,7 @@ package catalog
 
 import (
 	"context"
+	"fmt"
 	"runtime"
 
 	"golang.org/x/sync/errgroup"
@@ -35,7 +36,10 @@ func runRedesign(ctx context.Context, l log.Logger, opts *options.TerragruntOpti
 			svc := catalog.NewCatalogService(opts)
 
 			onModule := func(mod *module.Module) {
-				moduleCh <- mod
+				select {
+				case moduleCh <- mod:
+				case <-ctx.Done():
+				}
 			}
 
 			urlCh := make(chan string, 10) //nolint:mnd
@@ -74,6 +78,8 @@ func runRedesign(ctx context.Context, l log.Logger, opts *options.TerragruntOpti
 
 				loaders.Go(func() error {
 					if err := svc.LoadStreamingURL(loadCtx, l, repoURL, onModule); err != nil {
+						// Individual repo failures are non-critical — warn and
+						// continue so remaining repos can still load.
 						l.Warnf("Error loading %s: %v", repoURL, err)
 					}
 
@@ -82,11 +88,11 @@ func runRedesign(ctx context.Context, l log.Logger, opts *options.TerragruntOpti
 			}
 
 			if err := loaders.Wait(); err != nil {
-				l.Warnf("Loader error: %v", err)
+				return nil, fmt.Errorf("loading modules: %w", err)
 			}
 
 			if err := g.Wait(); err != nil {
-				l.Warnf("Discovery error: %v", err)
+				return nil, fmt.Errorf("discovering sources: %w", err)
 			}
 
 			if len(svc.Modules()) == 0 {

internal/cli/commands/catalog/tui/keys.go

@@ -78,7 +78,7 @@ type DelegateKeyMap struct {
 	Scaffold key.Binding
 }
 
-// Additional short help entries. This satisfies the help.KeyMap interface and
+// ShortHelp returns additional short help entries. This satisfies the help.KeyMap interface and
 // is entirely optional.
 func (d DelegateKeyMap) ShortHelp() []key.Binding { //nolint:gocritic
 	return []key.Binding{
@@ -87,7 +87,7 @@ func (d DelegateKeyMap) ShortHelp() []key.Binding { //nolint:gocritic
 	}
 }
 
-// Additional full help entries. This satisfies the help.KeyMap interface and
+// FullHelp returns additional full help entries. This satisfies the help.KeyMap interface and
 // is entirely optional.
 func (d DelegateKeyMap) FullHelp() [][]key.Binding { //nolint:gocritic
 	return [][]key.Binding{

internal/cli/commands/catalog/tui/redesign/model.go

@@ -1,3 +1,5 @@
+// Package redesign implements the redesigned catalog TUI experience with
+// streaming module discovery and a welcome loading screen.
 package redesign
 
 import (
@@ -142,11 +144,14 @@ func newModelWithItems(l log.Logger, opts *options.TerragruntOptions, svc catalo
 // skipping duplicates. If the user has started navigating, the cursor stays
 // on the currently selected item. Otherwise it stays at the top of the list.
 func (m *Model) insertModuleSorted(mod *module.Module) tea.Cmd {
+	if mod == nil {
+		return nil
+	}
+
 	items := m.List.Items()
 	modTitle := mod.Title()
 
-	// Binary search finds the insertion point and doubles as a duplicate check:
-	// if the item at insertIdx matches, the module is already in the list.
+	// Binary search finds the insertion point by title for sort order.
 	insertIdx := sort.Search(len(items), func(i int) bool {
 		if existing, ok := items[i].(*module.Module); ok {
 			return strings.ToLower(existing.Title()) >= strings.ToLower(modTitle)
@@ -155,7 +160,9 @@ func (m *Model) insertModuleSorted(mod *module.Module) tea.Cmd {
 		return false
 	})
 
-	if isDuplicate(items, insertIdx, modTitle) {
+	// De-duplicate by source path, not title, so distinct modules that
+	// share a display name are not collapsed.
+	if isDuplicate(items, mod.TerraformSourcePath()) {
 		return nil
 	}
 
@@ -177,19 +184,20 @@ func (m *Model) insertModuleSorted(mod *module.Module) tea.Cmd {
 	return cmd
 }
 
-// isDuplicate reports whether the item at idx in the sorted list has the
-// same title (case-insensitive) as modTitle.
-func isDuplicate(items []list.Item, idx int, modTitle string) bool {
-	if idx >= len(items) {
-		return false
-	}
-
-	existing, ok := items[idx].(*module.Module)
-	if !ok {
-		return false
+// isDuplicate reports whether any item in the list has the same source path
+// as sourcePath. This uses the stable TerraformSourcePath identity rather
+// than the display title, so distinct modules that share a title are not
+// incorrectly collapsed.
+func isDuplicate(items []list.Item, sourcePath string) bool {
+	for _, item := range items {
+		if existing, ok := item.(*module.Module); ok {
+			if existing.TerraformSourcePath() == sourcePath {
+				return true
+			}
+		}
 	}
 
-	return strings.EqualFold(existing.Title(), modTitle)
+	return false
 }
 
 func (m Model) listenForModule() tea.Cmd { //nolint:gocritic

internal/cli/commands/catalog/tui/redesign/model_test.go

@@ -77,11 +77,11 @@ func createMockCatalogService(t *testing.T, opts *options.TerragruntOptions) cat
 		repoURL := repoOpts.CloneURL
 		dummyRepoDir := filepath.Join(helpers.TmpDirWOSymlinks(t), strings.ReplaceAll(repoURL, "github.com/gruntwork-io/", ""))
 
-		os.MkdirAll(dummyRepoDir, 0755)
+		require.NoError(t, os.MkdirAll(dummyRepoDir, 0755), "MkdirAll %s", dummyRepoDir)
 
 		gitDir := filepath.Join(dummyRepoDir, ".git")
-		os.MkdirAll(gitDir, 0755)
-		os.WriteFile(filepath.Join(gitDir, "config"), fmt.Appendf(nil, `[core]
+		require.NoError(t, os.MkdirAll(gitDir, 0755), "MkdirAll %s", gitDir)
+		require.NoError(t, os.WriteFile(filepath.Join(gitDir, "config"), fmt.Appendf(nil, `[core]
 	repositoryformatversion = 0
 	filemode = true
 	bare = false
@@ -92,29 +92,33 @@ func createMockCatalogService(t *testing.T, opts *options.TerragruntOptions) cat
 [branch "main"]
 	remote = origin
 	merge = refs/heads/main
-`, repoURL), 0644)
-		os.WriteFile(filepath.Join(gitDir, "HEAD"), []byte("ref: refs/heads/main\n"), 0644)
+`, repoURL), 0644), "WriteFile %s/config", gitDir)
+		require.NoError(t, os.WriteFile(filepath.Join(gitDir, "HEAD"), []byte("ref: refs/heads/main\n"), 0644), "WriteFile %s/HEAD", gitDir)
 
 		refsDir := filepath.Join(gitDir, "refs")
 		headsDir := filepath.Join(refsDir, "heads")
 		remotesDir := filepath.Join(refsDir, "remotes", "origin")
 
-		os.MkdirAll(headsDir, 0755)
-		os.MkdirAll(remotesDir, 0755)
+		require.NoError(t, os.MkdirAll(headsDir, 0755), "MkdirAll %s", headsDir)
+		require.NoError(t, os.MkdirAll(remotesDir, 0755), "MkdirAll %s", remotesDir)
 
 		fakeCommitHash := "1234567890abcdef1234567890abcdef12345678"
-		os.WriteFile(filepath.Join(headsDir, "main"), []byte(fakeCommitHash+"\n"), 0644)
-		os.WriteFile(filepath.Join(remotesDir, "main"), []byte(fakeCommitHash+"\n"), 0644)
+		require.NoError(t, os.WriteFile(filepath.Join(headsDir, "main"), []byte(fakeCommitHash+"\n"), 0644), "WriteFile %s/main", headsDir)
+		require.NoError(t, os.WriteFile(filepath.Join(remotesDir, "main"), []byte(fakeCommitHash+"\n"), 0644), "WriteFile %s/main", remotesDir)
 
 		switch repoURL {
 		case "github.com/gruntwork-io/test-repo-1":
 			readme1Path := filepath.Join(dummyRepoDir, "README.md")
-			os.WriteFile(readme1Path, []byte("# AWS VPC Module\nThis module creates a VPC in AWS with all the necessary components."), 0644)
-			os.WriteFile(filepath.Join(dummyRepoDir, "main.tf"), []byte("# VPC terraform configuration"), 0644)
+			require.NoError(t, os.WriteFile(readme1Path, []byte("# AWS VPC Module\nThis module creates a VPC in AWS with all the necessary components."), 0644), "WriteFile %s", readme1Path)
+
+			mainTF1 := filepath.Join(dummyRepoDir, "main.tf")
+			require.NoError(t, os.WriteFile(mainTF1, []byte("# VPC terraform configuration"), 0644), "WriteFile %s", mainTF1)
 		case "github.com/gruntwork-io/test-repo-2":
 			readme2Path := filepath.Join(dummyRepoDir, "README.md")
-			os.WriteFile(readme2Path, []byte("# AWS EKS Module\nThis module creates an EKS cluster in AWS."), 0644)
-			os.WriteFile(filepath.Join(dummyRepoDir, "main.tf"), []byte("# EKS terraform configuration"), 0644)
+			require.NoError(t, os.WriteFile(readme2Path, []byte("# AWS EKS Module\nThis module creates an EKS cluster in AWS."), 0644), "WriteFile %s", readme2Path)
+
+			mainTF2 := filepath.Join(dummyRepoDir, "main.tf")
+			require.NoError(t, os.WriteFile(mainTF2, []byte("# EKS terraform configuration"), 0644), "WriteFile %s", mainTF2)
 		default:
 			return nil, fmt.Errorf("unexpected repoURL in mock: %s", repoURL)
 		}
@@ -135,7 +139,7 @@ func createMockCatalogService(t *testing.T, opts *options.TerragruntOptions) cat
 	require.NoError(t, err)
 
 	unitDir := filepath.Join(tmpDir, "unit")
-	os.MkdirAll(unitDir, 0755)
+	require.NoError(t, os.MkdirAll(unitDir, 0755), "MkdirAll %s", unitDir)
 	opts.TerragruntConfigPath = filepath.Join(unitDir, "terragrunt.hcl")
 	opts.ScaffoldRootFileName = config.RecommendedParentConfigName
 
@@ -182,10 +186,10 @@ func TestModelStreamingInsertsSorted(t *testing.T) {
 	items := finalModel.List.Items()
 	assert.Len(t, items, len(modules), "all modules should be in the list")
 
-	// Verify sorted order
+	// Verify sorted order (case-insensitive, matching the sort in model.go)
 	for i := 1; i < len(items); i++ {
-		prev := items[i-1].(*module.Module).Title()
-		curr := items[i].(*module.Module).Title()
+		prev := strings.ToLower(items[i-1].(*module.Module).Title())
+		curr := strings.ToLower(items[i].(*module.Module).Title())
 		assert.LessOrEqual(t, prev, curr, "modules should be in alphabetical order: %q should come before %q", prev, curr)
 	}
 }

internal/cli/commands/catalog/tui/redesign/update.go

@@ -92,7 +92,7 @@ func updateList(msg tea.Msg, m Model) (tea.Model, tea.Cmd) { //nolint:gocritic
 					m.State = PagerState
 				case key.Matches(msg, m.delegateKeys.Scaffold):
 					if m.SVC == nil {
-						break
+						return m, nil
 					}
 
 					m.State = ScaffoldState
@@ -114,7 +114,7 @@ func updateList(msg tea.Msg, m Model) (tea.Model, tea.Cmd) { //nolint:gocritic
 
 	// Append any commands from button bar initialization
 	if len(cmds) > 0 {
-		return m, tea.Batch(cmd, tea.Batch(cmds...))
+		return m, tea.Batch(append([]tea.Cmd{cmd}, cmds...)...)
 	}
 
 	return m, cmd
@@ -146,7 +146,7 @@ func updatePager(msg tea.Msg, m Model) (tea.Model, tea.Cmd) { //nolint:gocritic
 			switch currentAction {
 			case scaffoldBtn:
 				if m.SVC == nil {
-					break
+					return m, nil
 				}
 
 				m.State = ScaffoldState
@@ -164,7 +164,7 @@ func updatePager(msg tea.Msg, m Model) (tea.Model, tea.Cmd) { //nolint:gocritic
 
 		case key.Matches(msg, m.pagerKeys.Scaffold):
 			if m.SVC == nil {
-				break
+				return m, nil
 			}
 
 			m.State = ScaffoldState

internal/cli/commands/catalog/tui/redesign/view.go

@@ -11,7 +11,7 @@ import (
 var (
 	AppStyle          = lipgloss.NewStyle().Padding(1, 2) //nolint:mnd
 	infoPositionStyle = lipgloss.NewStyle().Padding(0, 1).BorderStyle(lipgloss.HiddenBorder())
-	infoLineStyle     = lipgloss.NewStyle().Foreground(lipgloss.Color("#1D252"))
+	infoLineStyle     = lipgloss.NewStyle().Foreground(lipgloss.Color("#1D252F"))
 	infoHelp          = lipgloss.NewStyle().Padding(2, 0, 0, 2) //nolint:mnd
 )
 

internal/cli/commands/catalog/tui/redesign/welcome.go

@@ -35,6 +35,7 @@ type welcomeState int
 const (
 	welcomeLoading welcomeState = iota
 	welcomeNoSources
+	welcomeDiscoveryError
 )
 
 // DiscoveryCompleteMsg is sent when background discovery finishes.
@@ -79,18 +80,19 @@ var (
 // discovery runs in the background, then either transitions to the module
 // list TUI or settles into a "no sources found" help screen.
 type WelcomeModel struct {
-	ctx        context.Context
-	logger     log.Logger
-	opts       *options.TerragruntOptions
-	loadFunc   LoadFunc
-	openURL    OpenURLFunc
-	statusCh   chan string
-	moduleCh   chan *module.Module
-	statusText string
-	spinner    spinner.Model
-	state      welcomeState
-	width      int
-	height     int
+	ctx              context.Context
+	logger           log.Logger
+	lastDiscoveryErr error
+	moduleCh         chan *module.Module
+	openURL          OpenURLFunc
+	statusCh         chan string
+	loadFunc         LoadFunc
+	opts             *options.TerragruntOptions
+	statusText       string
+	spinner          spinner.Model
+	state            welcomeState
+	width            int
+	height           int
 }
 
 // NewWelcomeModel creates a WelcomeModel that immediately begins discovery.
@@ -224,6 +226,10 @@ func (m WelcomeModel) handleModuleMsg(msg moduleMsg) (tea.Model, tea.Cmd) { //no
 func (m WelcomeModel) handleDiscoveryComplete(msg DiscoveryCompleteMsg) (tea.Model, tea.Cmd) { //nolint:gocritic
 	if msg.Err != nil {
 		m.logger.Warnf("Discovery error: %v", msg.Err)
+		m.lastDiscoveryErr = msg.Err
+		m.state = welcomeDiscoveryError
+
+		return m, nil
 	}
 
 	// Defensive: if we're still on the loading screen but the service has
@@ -256,6 +262,8 @@ func (m WelcomeModel) View() tea.View { //nolint:gocritic
 		content = m.loadingView()
 	case welcomeNoSources:
 		content = m.noSourcesView()
+	case welcomeDiscoveryError:
+		content = m.discoveryErrorView()
 	}
 
 	if m.width > 0 && m.height > 0 {
@@ -303,6 +311,29 @@ func (m WelcomeModel) noSourcesView() string { //nolint:gocritic
 	return lipgloss.JoinVertical(lipgloss.Center, title, body)
 }
 
+func (m WelcomeModel) discoveryErrorView() string { //nolint:gocritic
+	title := welcomeTitleStyle.Render(" Terragrunt Catalog ")
+
+	errMsg := "unknown error"
+	if m.lastDiscoveryErr != nil {
+		errMsg = m.lastDiscoveryErr.Error()
+	}
+
+	body := welcomeBodyStyle.Render(lipgloss.JoinVertical(lipgloss.Left,
+		"",
+		"An error occurred while discovering catalog sources:",
+		"",
+		welcomeCodeStyle.Render("  "+errMsg),
+		"",
+		"Please check your network connection, authentication, and",
+		"catalog configuration, then try again.",
+		"",
+		welcomeHintStyle.Render("q/esc: exit"),
+	))
+
+	return lipgloss.JoinVertical(lipgloss.Center, title, body)
+}
+
 // RunRedesign launches the redesigned catalog experience. It shows a loading
 // screen immediately while discovery runs in the background, then transitions
 // to the module list if modules are found.

internal/cli/commands/catalog/tui/redesign/welcome_test.go

@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"context"
 	"os"
+	"strings"
 	"testing"
 	"time"
 
@@ -270,11 +271,11 @@ func TestWelcomeStreamingModules(t *testing.T) {
 	assert.Equal(t, redesign.ListState, listModel.State)
 	assert.Len(t, listModel.List.Items(), len(modules), "all streamed modules should appear in list")
 
-	// Verify alphabetical order
+	// Verify alphabetical order (case-insensitive, matching the sort in model.go)
 	items := listModel.List.Items()
 	for i := 1; i < len(items); i++ {
-		prev := items[i-1].(*module.Module).Title()
-		curr := items[i].(*module.Module).Title()
+		prev := strings.ToLower(items[i-1].(*module.Module).Title())
+		curr := strings.ToLower(items[i].(*module.Module).Title())
 		assert.LessOrEqual(t, prev, curr, "modules should be in alphabetical order")
 	}
 }

internal/cli/commands/catalog/tui/update.go

@@ -90,7 +90,7 @@ func updateList(msg tea.Msg, m Model) (tea.Model, tea.Cmd) { //nolint:gocritic
 					m.State = PagerState
 				case key.Matches(msg, m.delegateKeys.Scaffold):
 					if m.SVC == nil {
-						break
+						return m, nil
 					}
 
 					m.State = ScaffoldState
@@ -144,7 +144,7 @@ func updatePager(msg tea.Msg, m Model) (tea.Model, tea.Cmd) { //nolint:gocritic
 			switch currentAction {
 			case scaffoldBtn:
 				if m.SVC == nil {
-					break
+					return m, nil
 				}
 
 				m.State = ScaffoldState
@@ -162,7 +162,7 @@ func updatePager(msg tea.Msg, m Model) (tea.Model, tea.Cmd) { //nolint:gocritic
 
 		case key.Matches(msg, m.pagerKeys.Scaffold):
 			if m.SVC == nil {
-				break
+				return m, nil
 			}
 
 			m.State = ScaffoldState