refactor: [#107] centralize UserOutput via dependency injection #108

josecelano · 2025-10-31T16:35:42Z

Summary

This PR centralizes UserOutput management via dependency injection, providing better control over output formatting and reducing duplication throughout the codebase.

Changes

Core Changes

✅ Add AppContainer for centralized dependency management (src/bootstrap/container.rs)
✅ Update all command handlers to accept &Arc<Mutex<UserOutput>>
✅ Remove UserOutput cloning throughout command execution
✅ Ensure thread-safe shared ownership of output stream

Documentation

✅ Update all documentation examples to use Arc<Mutex<UserOutput>>
✅ Fix all doc tests to compile with new API (234 passing)

Testing

✅ Update all tests to use the new pattern
✅ All pre-commit checks passing

Benefits

Centralized Control: Single point of creation for UserOutput
Thread Safety: Proper Arc<Mutex<>> wrapper ensures safe concurrent access
Reduced Duplication: Eliminates cloning throughout the codebase
Better Testing: Easier to inject mock outputs for testing

Files Changed (20 files)

src/bootstrap/app.rs - Updated to use AppContainer
src/bootstrap/container.rs - NEW - Dependency injection container
src/bootstrap/mod.rs - Module updates
src/presentation/commands/context.rs - Updated API and docs
src/presentation/commands/create/* - Updated create command handlers
src/presentation/commands/destroy/* - Updated destroy command handlers
src/presentation/commands/factory.rs - Updated factory methods
src/presentation/commands/mod.rs - Updated command execution
src/presentation/progress.rs - Updated progress reporting
Tests updated across all command modules

Status

All tests passing
Documentation updated
Pre-commit checks passing
Additional refactors planned (keeping as draft)

- Add AppContainer for centralized dependency management - Update all command handlers to accept &Arc<Mutex<UserOutput>> - Remove UserOutput cloning throughout command execution - Update all documentation examples to use Arc<Mutex<UserOutput>> - Ensure thread-safe shared ownership of output stream This centralizes UserOutput creation and management, providing better control over output formatting and reducing duplication.

- Remove duplicate context creation in handle_environment_creation - Remove duplicate context creation in handle_destroy_command - Remove unused into_user_output method from CommandContext - Clone Arc<Mutex<UserOutput>> instead of consuming context This improves efficiency by reusing the context throughout command execution instead of recreating it unnecessarily.

- Remove report_error function from CommandContext - Call UserOutput::error() directly where needed - Simplifies error reporting by eliminating indirection - All error reporting now uses consistent pattern: user_output.lock().unwrap().error() This reduces unnecessary abstraction and makes the code more straightforward by calling the UserOutput service directly.

- Add create_test_setup() helper function in context.rs tests - Refactor 7 test functions to use the helper instead of repeated setup code - Reduces 21 lines of duplicated setup to 7 lines using helper - Fix clippy warnings: add backticks to doc comments and Panics section - Improves test maintainability by centralizing test setup logic

- Add create_test_setup() helper function in factory.rs tests - Refactor 6 test functions to use helper instead of repeated setup - Reduces 24 lines of duplicated setup code to 6 lines using helper - Improves test maintainability by centralizing factory test setup

- Created ProgressReporterError enum for progress reporting failures - Updated ProgressReporter methods to return Result<(), ProgressReporterError> - Added ProgressReportingFailed variant to DestroySubcommandError and CreateSubcommandError - Replaced all .expect() calls with .map_err() for proper error propagation - Removed problematic 'if let Ok(mut output)' patterns that silently ignored errors - Updated all 26 test cases to handle Result returns with .expect() - Added comprehensive help text for all new error variants - Updated documentation with # Errors sections and Result examples This eliminates all mutex poisoning panics, ensuring errors are properly propagated through the call stack with full traceability and actionable error messages for users.

…ReporterError - Add From<ProgressReporterError> implementations for DestroySubcommandError and CreateSubcommandError - Replace verbose .map_err(|e| Error::ProgressReportingFailed { source: e }) with simple ? operator - Eliminates ~80 lines of boilerplate across destroy and create handlers - Follows idiomatic Rust patterns for automatic error conversion

Copilot

Pull Request Overview

This PR refactors the UserOutput service to use thread-safe shared ownership via Arc<Mutex<UserOutput>> instead of direct ownership, enabling consistent output across concurrent operations and multiple components. This architectural change addresses concurrency concerns and establishes a centralized service container pattern for dependency injection.

Key Changes:

Introduced Arc<Mutex<UserOutput>> wrapper pattern for thread-safe shared access to user output
Added ProgressReporterError enum with proper error handling for mutex poisoning
Created Container service container in bootstrap layer for centralized dependency injection
Updated all command handlers and progress reporting to use the new shared output pattern

Reviewed Changes

Copilot reviewed 20 out of 20 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
`src/presentation/progress.rs`	Refactored `ProgressReporter` to accept `Arc<Mutex<UserOutput>>`, added error handling for mutex operations, updated all methods to return `Result`
`src/presentation/errors.rs`	Added `UserOutputLockFailed` error variant to `CommandError` with comprehensive help text
`src/presentation/commands/mod.rs`	Updated `execute` and `handle_error` functions to accept shared `UserOutput` reference, added fallback error reporting
`src/presentation/commands/factory.rs`	Modified `create_context` to require `user_output` parameter, updated all factory methods and tests
`src/presentation/commands/context.rs`	Replaced owned `UserOutput` with `Arc<Mutex<UserOutput>>`, removed `into_output()` and `report_error()` functions, updated tests
`src/presentation/commands/create/handler.rs`	Updated to pass `user_output` through to subcommand handlers
`src/presentation/commands/create/subcommands/environment.rs`	Refactored to use shared `UserOutput`, updated error reporting with mutex lock attempts
`src/presentation/commands/create/subcommands/template.rs`	Updated to accept and lock `user_output` parameter
`src/presentation/commands/create/errors.rs`	Added `UserOutputLockFailed` and `ProgressReportingFailed` error variants with detailed help
`src/presentation/commands/destroy/handler.rs`	Updated to use shared `UserOutput`, removed inline error reporting
`src/presentation/commands/destroy/errors.rs`	Added mutex poisoning and progress reporting error variants with comprehensive help
`src/presentation/commands/tests/mod.rs`	Added `user_output` field to `TestContext` for testing
`src/bootstrap/container.rs`	New file: Implements service container for centralized dependency injection
`src/bootstrap/app.rs`	Integrated `Container` for initializing and providing shared services to commands

Copilot · 2025-10-31T18:29:28Z

src/presentation/commands/create/subcommands/environment.rs

-    ));
+    user_output
+        .lock()
+        .map_err(|_| crate::presentation::progress::ProgressReporterError::UserOutputMutexPoisoned)?


This error mapping is converting a mutex poisoning error to ProgressReporterError::UserOutputMutexPoisoned, but the function returns CreateSubcommandError. This works because there's a From implementation, but it's semantically incorrect - this is not a progress reporting error, it's occurring during configuration loading. Consider mapping to CreateSubcommandError::UserOutputLockFailed directly instead, or create a helper function to avoid the confusing intermediate conversion.

Suggested change

.map_err(|_| crate::presentation::progress::ProgressReporterError::UserOutputMutexPoisoned)?

.map_err(|_| CreateSubcommandError::UserOutputLockFailed)?

Copilot · 2025-10-31T18:29:28Z

src/presentation/commands/create/subcommands/environment.rs

-    output.progress("Validating configuration and creating environment...");
+    user_output
+        .lock()
+        .map_err(|_| crate::presentation::progress::ProgressReporterError::UserOutputMutexPoisoned)?


Same issue as Comment 1 - these mutex lock failures in execute_create_command are being mapped to ProgressReporterError::UserOutputMutexPoisoned when they should map to CreateSubcommandError::UserOutputLockFailed. The semantic mismatch makes error tracing and debugging more difficult. These are not progress reporting errors, they're user output access errors during command execution.

Copilot · 2025-10-31T18:29:28Z

src/presentation/commands/create/subcommands/environment.rs

+
+    user_output
+        .lock()
+        .map_err(|_| crate::presentation::progress::ProgressReporterError::UserOutputMutexPoisoned)?


Same issue as Comment 1 - these mutex lock failures in execute_create_command are being mapped to ProgressReporterError::UserOutputMutexPoisoned when they should map to CreateSubcommandError::UserOutputLockFailed. The semantic mismatch makes error tracing and debugging more difficult. These are not progress reporting errors, they're user output access errors during command execution.

Copilot · 2025-10-31T18:29:28Z

src/presentation/commands/create/subcommands/environment.rs

+/// # Panics
+///
+/// This function will panic if the `UserOutput` mutex is poisoned. Since this is
+/// called after successful environment creation (when operation is complete),
+/// a poisoned mutex indicates an irrecoverable state and panicking is acceptable.


The panic documentation is technically accurate but philosophically inconsistent with the rest of the PR's error handling approach. Throughout the PR, mutex poisoning is carefully handled with proper error types and user-friendly messages. Here, it's allowed to panic with expect() at line 205. While the justification is reasonable (operation already succeeded), consider whether this should return a Result instead for consistency, or at least use a more informative panic message that matches the help text from the error variants.

Copilot · 2025-10-31T18:29:29Z

src/presentation/commands/destroy/handler.rs


    // Create progress reporter for 3 main steps
-    let mut progress = ProgressReporter::new(ctx.into_output(), 3);
+    let mut progress = ProgressReporter::new(user_output.clone(), 3);


The user_output is cloned here and then immediately cloned again on line 68 for the ProgressReporter. Since Arc::clone() is cheap (just incrementing a reference count), this is not a major issue, but it's unnecessary - the second clone could be avoided by passing ctx.user_output() to ProgressReporter::new() instead of user_output.clone(). This would make the ownership flow clearer and eliminate one redundant clone.

Suggested change

let mut progress = ProgressReporter::new(user_output.clone(), 3);

let mut progress = ProgressReporter::new(ctx.user_output(), 3);

Copilot · 2025-10-31T18:29:29Z

src/presentation/commands/create/subcommands/environment.rs


    // Create progress reporter for 3 main steps
-    let mut progress = ProgressReporter::new(ctx.into_output(), 3);
+    let mut progress = ProgressReporter::new(user_output.clone(), 3);


Same performance issue as Comment 4 - user_output is cloned twice unnecessarily. The second clone for ProgressReporter::new() could use ctx.user_output() instead, eliminating a redundant Arc::clone() operation and making the ownership flow clearer.

Suggested change

let mut progress = ProgressReporter::new(user_output.clone(), 3);

let mut progress = ProgressReporter::new(ctx.user_output(), 3);

Copilot · 2025-10-31T18:29:29Z

src/presentation/commands/mod.rs

+    if let Ok(mut output) = user_output.lock() {
+        output.error(&format!("{error}"));
+        output.blank_line();
+        output.info_block("For detailed troubleshooting:", &[help_text]);
+    } else {
+        // Cannot acquire lock - print to stderr directly as fallback
+        eprintln!("ERROR: {error}");
+        eprintln!();
+        eprintln!("CRITICAL: Failed to acquire user output lock.");
+        eprintln!("This indicates a panic occurred in another thread.");
+        eprintln!();
+        eprintln!("For detailed troubleshooting:");
+        eprintln!("{help_text}");
+    }


[nitpick] The fallback error handling using eprintln! directly is a good defensive programming practice. However, the fallback messages lack the same formatting and visual indicators (emojis, styling) that the normal UserOutput provides. Consider whether the fallback should attempt to maintain consistent formatting, or if this plain text approach is intentional for the degraded error state. Document the rationale in a comment if the plain text is intentional.

- Fix error mapping: Use CreateSubcommandError::UserOutputLockFailed instead of ProgressReporterError::UserOutputMutexPoisoned - Eliminate unnecessary clones: Use ctx.user_output().clone() directly for ProgressReporter initialization - Enhance panic message with detailed context about UserOutput lock poisoning - Document fallback error formatting rationale in handle_error() All changes address PR #108 review comments from GitHub Copilot.

…ethods - Create DestroyCommandController struct to encapsulate workflow orchestration - Extract private step methods: validate_environment_name, initialize_dependencies, tear_down_infrastructure, complete_workflow - Add DESTROY_WORKFLOW_STEPS constant for step count - Simplify handle_destroy_command to be a thin wrapper over controller - Improve testability by separating concerns between presentation and application layers - Add comprehensive documentation with # Errors sections for public methods This refactor improves code clarity, maintainability, and testability while maintaining backward compatibility with existing tests.

josecelano · 2025-10-31T19:23:02Z

ACK 75af108

josecelano self-assigned this Oct 31, 2025

josecelano added the refactoring label Oct 31, 2025

josecelano added 6 commits October 31, 2025 16:45

josecelano marked this pull request as ready for review October 31, 2025 18:24

josecelano requested a review from Copilot October 31, 2025 18:24

Copilot AI reviewed Oct 31, 2025

View reviewed changes

josecelano added 2 commits October 31, 2025 18:58

josecelano merged commit 79cd8ea into main Oct 31, 2025
34 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor: [#107] centralize UserOutput via dependency injection #108

refactor: [#107] centralize UserOutput via dependency injection #108

Uh oh!

josecelano commented Oct 31, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Oct 31, 2025

Uh oh!

Copilot AI Oct 31, 2025

Uh oh!

Copilot AI Oct 31, 2025

Uh oh!

Copilot AI Oct 31, 2025

Uh oh!

Copilot AI Oct 31, 2025

Uh oh!

Copilot AI Oct 31, 2025

Uh oh!

Copilot AI Oct 31, 2025

Uh oh!

josecelano commented Oct 31, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	.map_err(\|_\| crate::presentation::progress::ProgressReporterError::UserOutputMutexPoisoned)?
	.map_err(\|_\| CreateSubcommandError::UserOutputLockFailed)?

	let mut progress = ProgressReporter::new(user_output.clone(), 3);
	let mut progress = ProgressReporter::new(ctx.user_output(), 3);

refactor: [#107] centralize UserOutput via dependency injection #108

refactor: [#107] centralize UserOutput via dependency injection #108

Uh oh!

Conversation

josecelano commented Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Core Changes

Documentation

Testing

Benefits

Files Changed (20 files)

Status

Related

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Key Changes:

Reviewed Changes

Uh oh!

Copilot AI Oct 31, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 31, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 31, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 31, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 31, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 31, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 31, 2025

Choose a reason for hiding this comment

Uh oh!

josecelano commented Oct 31, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

josecelano commented Oct 31, 2025 •

edited

Loading