fix: Addressing review feedback

Author: yhakbar

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

@@ -43,35 +43,58 @@ func (c *CopyCmd) Run() error {
 
 	c.logger.Debugf("Copying component %q to %q", src, dst)
 
-	if err := copyDir(src, dst); err != nil {
+	// Preflight: refuse before writing anything if any target file would
+	// collide with something already in the working directory. Without this,
+	// a mid-walk collision could leave the working tree in a half-copied
+	// state.
+	if err := preflightCopy(src, dst); err != nil {
 		return err
 	}
 
-	c.result.workingDir = dst
-
 	configName := configFileForKind(c.component.Kind)
-	if configName == "" {
-		return nil
+
+	var (
+		refs    ValuesReferences
+		hasRefs bool
+	)
+
+	if configName != "" {
+		refs, err = CollectValuesReferences(filepath.Join(src, configName))
+		if err != nil {
+			return err
+		}
+
+		hasRefs = !refs.IsEmpty()
+
+		// Also preflight the values stub destination so we can fail before
+		// copying when a stub would be written but the destination has an
+		// unrelated obstruction (e.g. it exists as a directory).
+		if hasRefs {
+			if err := preflightValuesStub(dst); err != nil {
+				return err
+			}
+		}
 	}
 
-	refs, err := CollectValuesReferences(filepath.Join(src, configName))
-	if err != nil {
+	if err := copyDir(src, dst); err != nil {
 		return err
 	}
 
-	if refs.IsEmpty() {
-		return nil
-	}
+	result := copyResult{workingDir: dst}
 
-	c.result.references = refs
+	if hasRefs {
+		result.references = refs
 
-	written, err := WriteValuesStub(dst, refs)
-	if err != nil {
-		return err
+		written, err := WriteValuesStub(dst, refs)
+		if err != nil {
+			return err
+		}
+
+		result.valuesWritten = written
+		result.valuesSkipped = !written
 	}
 
-	c.result.valuesWritten = written
-	c.result.valuesSkipped = !written
+	c.result = result
 
 	return nil
 }
@@ -82,8 +105,14 @@ func (c *CopyCmd) Result() copyResult {
 	return c.result
 }
 
-func (c *CopyCmd) SetStdin(io.Reader)  {}
+// SetStdin is a no-op; CopyCmd does not interact with stdio and only
+// implements this method to satisfy the tea.ExecCommand interface.
+func (c *CopyCmd) SetStdin(io.Reader) {}
+
+// SetStdout is a no-op; see SetStdin.
 func (c *CopyCmd) SetStdout(io.Writer) {}
+
+// SetStderr is a no-op; see SetStdin.
 func (c *CopyCmd) SetStderr(io.Writer) {}
 
 // resolvePaths returns the absolute source directory (inside the cloned repo)
@@ -157,7 +186,69 @@ func copyDir(src, dst string) error {
 	})
 }
 
-func copyFile(src, dst string) error {
+// preflightCopy walks src and returns an error if any non-skipped regular
+// file would land on a path that already exists in dst. This makes the copy
+// step all-or-nothing for the common collision case, so a half-populated
+// working directory cannot result from a mid-walk conflict.
+func preflightCopy(src, dst string) error {
+	return filepath.WalkDir(src, func(path string, d fs.DirEntry, walkErr error) error {
+		if walkErr != nil {
+			return walkErr
+		}
+
+		if d.IsDir() {
+			if path != src && skipDuringCopy(d.Name()) {
+				return filepath.SkipDir
+			}
+
+			return nil
+		}
+
+		if !d.Type().IsRegular() {
+			return nil
+		}
+
+		rel, err := filepath.Rel(src, path)
+		if err != nil {
+			return errors.New(err)
+		}
+
+		target := filepath.Join(dst, rel)
+		if _, err := os.Lstat(target); err == nil {
+			return errors.Errorf("destination %q already exists; refusing to overwrite", target)
+		} else if !os.IsNotExist(err) {
+			return errors.New(err)
+		}
+
+		return nil
+	})
+}
+
+// preflightValuesStub returns an error if WriteValuesStub would fail at the
+// stub destination for any reason other than a pre-existing values file
+// (which it intentionally leaves alone).
+func preflightValuesStub(dst string) error {
+	stub := filepath.Join(dst, valuesFileName)
+
+	info, err := os.Lstat(stub)
+	if err != nil {
+		if os.IsNotExist(err) {
+			return nil
+		}
+
+		return errors.New(err)
+	}
+
+	// A regular file at the stub path is fine; WriteValuesStub will leave
+	// it alone. Anything else (directory, symlink, irregular) blocks us.
+	if info.Mode().IsRegular() {
+		return nil
+	}
+
+	return errors.Errorf("destination %q is not a regular file; refusing to overwrite", stub)
+}
+
+func copyFile(src, dst string) (err error) {
 	in, err := os.Open(src)
 	if err != nil {
 		return errors.New(err)

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

@@ -28,17 +28,22 @@ const (
 	templatePillBgS = "#3A2D55"
 	templatePillFgS = "#DDC4FA"
 
+	// Shared white foreground for the unit and stack pills, whose darker
+	// backgrounds always read against pure white in both selected and
+	// unselected states.
+	pillFgWhite = "#FFFFFF"
+
 	// Unit type pill (blue, matching the `list` / `find` command color).
 	unitPillBg  = "#1B46DD"
-	unitPillFg  = "#FFFFFF"
+	unitPillFg  = pillFgWhite
 	unitPillBgS = "#2E5BEA"
-	unitPillFgS = "#FFFFFF"
+	unitPillFgS = pillFgWhite
 
 	// Stack type pill (green, matching the `list` / `find` command color).
 	stackPillBg  = "#2E8B57"
-	stackPillFg  = "#FFFFFF"
+	stackPillFg  = pillFgWhite
 	stackPillBgS = "#3CA068"
-	stackPillFgS = "#FFFFFF"
+	stackPillFgS = pillFgWhite
 
 	// Version pill (neutral).
 	versionBg  = "#313244"

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

@@ -247,7 +247,12 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) {
 
 	case scaffoldFinishedMsg:
 		if msg.err != nil {
-			return m, tea.Batch(tea.Printf("error scaffolding component: %s", msg.err.Error()), tea.Quit)
+			// tea.Printf during alt-screen gets discarded on teardown, so
+			// stash the failure on the model and let RunRedesign emit it
+			// to the user's scrollback after exit.
+			m.exitMessage = formatActionFailure("scaffolding component", msg.err)
+
+			return m, tea.Quit
 		}
 
 		// Same post-exit-message pattern as the copy flow: stash a styled
@@ -259,7 +264,9 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) {
 
 	case copyFinishedMsg:
 		if msg.err != nil {
-			return m, tea.Batch(tea.Printf("error copying component: %s", msg.err.Error()), tea.Quit)
+			m.exitMessage = formatActionFailure("copying component", msg.err)
+
+			return m, tea.Quit
 		}
 
 		// Stash a styled post-exit message on the model so RunRedesign
@@ -330,10 +337,33 @@ func copyComponentCmd(l log.Logger, m Model, c *Component) tea.Cmd {
 const (
 	valuesBoxAccentGreen  = "#50FA7B"
 	valuesBoxAccentYellow = "#F1FA8C"
+	valuesBoxAccentRed    = "#FF5555"
 	valuesBoxPathColor    = "#8BE9FD"
 	valuesBoxMutedColor   = "#A8ACB1"
 )
 
+// formatActionFailure renders a bordered callout describing a failed
+// scaffold or copy action. action is a verb phrase ("scaffolding component",
+// "copying component"). The message is stashed on the model so RunRedesign
+// can print it after the alt screen is restored — tea.Printf lines emitted
+// during alt-screen are discarded on exit.
+func formatActionFailure(action string, err error) string {
+	heading := lipgloss.NewStyle().
+		Foreground(lipgloss.Color(valuesBoxAccentRed)).
+		Bold(true).
+		Render("error " + action)
+
+	body := err.Error()
+
+	content := lipgloss.JoinVertical(lipgloss.Left, heading, "", body)
+
+	return lipgloss.NewStyle().
+		Border(lipgloss.RoundedBorder()).
+		BorderForeground(lipgloss.Color(valuesBoxAccentRed)).
+		Padding(bodyPaddingVertical, bodyPaddingHorizontal).
+		Render(content)
+}
+
 var (
 	valuesBoxPathStyle = lipgloss.NewStyle().Foreground(lipgloss.Color(valuesBoxPathColor))
 	valuesBoxMuteStyle = lipgloss.NewStyle().Foreground(lipgloss.Color(valuesBoxMutedColor))

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

@@ -251,6 +251,9 @@ func WriteValuesStub(dir string, refs ValuesReferences) (bool, error) {
 
 	buf.WriteString(valuesStubHeader)
 
+	// Sort defensively so callers passing a hand-constructed
+	// ValuesReferences get deterministic output without needing to know that
+	// CollectValuesReferences happens to pre-sort.
 	if len(refs.Required) > 0 {
 		buf.WriteString(requiredSectionHeader)
 

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

@@ -147,11 +147,12 @@ func RunRedesign(ctx context.Context, l log.Logger, opts *options.TerragruntOpti
 	EmitExitMessage(finalModel, errWriter, l)
 
 	if err != nil {
-		if cause := context.Cause(ctx); errors.Is(cause, context.Canceled) {
+		cause := context.Cause(ctx)
+		if errors.Is(cause, context.Canceled) {
 			return nil
 		}
 
-		if cause := context.Cause(ctx); cause != nil {
+		if cause != nil {
 			return cause
 		}
 

internal/util/file.go

@@ -1104,14 +1104,18 @@ func SanitizePath(baseDir string, file string) (sanitized string, err error) {
 		}
 	}()
 
-	fileInfo, err := root.Stat(file)
-	if err != nil {
+	if _, err := root.Stat(file); err != nil {
 		return "", err
 	}
 
-	fullPath := baseDir + string(os.PathSeparator) + fileInfo.Name()
+	// Preserve nested directories from the validated input. Using
+	// fileInfo.Name() would flatten "a/b/c.txt" to "<baseDir>/c.txt".
+	// root.Stat already rejects paths that escape baseDir, so we only need
+	// to clean the input and join it back onto baseDir.
+	cleanedRelative := filepath.Clean(file)
+	cleanedRelative = strings.TrimLeft(cleanedRelative, string(os.PathSeparator))
 
-	return fullPath, nil
+	return filepath.Join(baseDir, cleanedRelative), nil
 }
 
 // RelPathForLog returns a relative path suitable for logging.

internal/util/file_test.go

@@ -612,7 +612,13 @@ func Test_sanitizePath(t *testing.T) {
 			name:    "happy path",
 			baseDir: "./testdata/fixture-sanitize-path/env/unit",
 			file:    ".terraform-version",
-			want:    "./testdata/fixture-sanitize-path/env/unit/.terraform-version",
+			want:    "testdata/fixture-sanitize-path/env/unit/.terraform-version",
+		},
+		{
+			name:    "nested file path is preserved",
+			baseDir: "./testdata/fixture-sanitize-path",
+			file:    "env/unit/.terraform-version",
+			want:    "testdata/fixture-sanitize-path/env/unit/.terraform-version",
 		},
 		{
 			name:    "base dir is empty",
@@ -646,7 +652,7 @@ func Test_sanitizePath(t *testing.T) {
 			name:    "file is just a dot",
 			baseDir: "./testdata/fixture-sanitize-path/env/unit",
 			file:    ".",
-			want:    "./testdata/fixture-sanitize-path/env/unit/.",
+			want:    "testdata/fixture-sanitize-path/env/unit",
 			wantErr: false,
 		},
 		{