refactor: extract BaseProgressDisplay class from progress displays

[ahogappa]

Summary

• Extract common progress display logic into BaseProgressDisplay class
• Refactor TreeProgressDisplay and SimpleProgressDisplay to inherit from base class
• Add new PlainProgressDisplay for CI/non-TTY environments (plain text output)

Changes

New BaseProgressDisplay class provides:

• Common task state management (TaskProgress, GroupProgress classes)
• Thread-safe operations with Monitor
• Shared utility methods (short_name, format_duration, tty?)
• Template method pattern for subclass customization

Template methods for subclasses:

• on_root_task_set - Called when root task is set
• on_section_impl_registered - Called when section impl is registered
• on_task_registered - Called when a task is registered
• on_task_updated - Called when task state changes
• on_group_updated - Called when group state changes
• should_activate? - Determine if display should activate
• on_start / on_stop - Lifecycle hooks

PlainProgressDisplay output format:

[TASKI] Starting RootTask
[START] TaskName
[DONE] TaskName (123.4ms)
[FAIL] TaskName: Error message
[TASKI] Completed: 5/5 tasks (1234ms)

Enable with: TASKI_PROGRESS_MODE=plain

Code reduction

• TreeProgressDisplay: ~200 lines removed (duplicated state management)
• SimpleProgressDisplay: ~100 lines removed
• Total: ~300 lines of duplicated code eliminated

Test plan

• All existing tests pass (306 tests, 815 assertions)
• Lint passes (rake standard)
• Manual testing of all three display modes

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Added a plain, non‑TTY progress display mode for simple text output and CI-friendly logs.

• Refactor

  • Introduced an extensible base progress display framework consolidating lifecycle hooks and thread‑safe progress tracking.
  • Updated existing progress displays to inherit from and integrate with the new base framework for consistent behavior and easier maintenance.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

Adds a BaseProgressDisplay scaffold, a PlainProgressDisplay implementation, and refactors SimpleProgressDisplay and TreeProgressDisplay to inherit and use the new base; also wires the new plain mode into the factory and env resolution. (48 words)

Changes

Cohort / File(s)	Summary
Framework Foundation lib/taski/execution/base_progress_display.rb	New BaseProgressDisplay with nested TaskProgress/GroupProgress, thread-safe APIs (register_task, update_task, update_group, start, stop), state transition logic, utilities, and protected hooks for subclasses.
Plain Display Implementation lib/taski/execution/plain_progress_display.rb	New PlainProgressDisplay < BaseProgressDisplay emitting plain-text lifecycle messages ([START],[DONE],[FAIL],[CLEAN], etc.), implements hooks (on_section_impl_registered, on_task_updated, on_start, on_stop, should_activate?), and flushes output after updates.
Simple Display Refactor lib/taski/execution/simple_progress_display.rb	Converted to inherit from BaseProgressDisplay, removed internal progress-tracking structs and synchronization, and migrated behavior to template hooks (on_root_task_set, on_section_impl_registered, on_start, on_stop).
Tree Display Refactor lib/taski/execution/tree_progress_display.rb	Converted to inherit from BaseProgressDisplay, removed local Monitor-based synchronization and internal state structs, and moved lifecycle and registration behavior to base-class hooks (on_root_task_set, on_section_impl_registered, on_task_registered, on_start, on_stop).
Mode Wiring lib/taski.rb	Added require 'taski/execution/plain_progress_display'; extended create_progress_display to return PlainProgressDisplay for :plain; extended progress_mode_from_env to accept "plain".

Sequence Diagram(s)

sequenceDiagram
  participant Factory as Taski.create_progress_display
  participant Display as PlainProgressDisplay
  participant Executor as Executor/Runner
  participant Out as STDERR/$stdout

  Factory->>Display: instantiate(output: STDERR)
  Executor->>Display: set_root_task(root_class)
  Executor->>Display: register_section_impl(section, impl)
  Executor->>Display: start()
  Executor->>Display: update_task(task_class, state: :running)
  Display->>Out: "[START] TaskName"
  Executor->>Display: update_task(task_class, state: :done, duration: 123)
  Display->>Out: "[DONE] TaskName (0.123s)"
  Executor->>Display: stop()
  Display->>Out: "[TASKI] Completed ... (summary)"

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Possibly related PRs

• feat: add group block for organizing progress display messages #105 — Adds/modifies group-level progress tracking (TaskProgress/GroupProgress) overlapping base progress data structures and update_group APIs.
• refactor: improve run_and_clean implementation and display #80 — Refactors progress-display architecture and lifecycle handling; directly related to base-class extraction and subclass changes.
• fix: improve output capture for progress display (#74) #76 — Integrates output-capture/set_output_capture semantics used by the new BaseProgressDisplay and display implementations.

Poem

🐰
I hopped in code with whiskers bright,
Built a base to show each task’s light,
Plain lines for stderr, trees still stand,
Progress marches on, by my fluffy hand,
Huzzah — go run, display tonight!

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 74.07% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main refactoring effort: extracting and centralizing common progress display logic into a new BaseProgressDisplay base class.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

---

🧹 Recent nitpick comments

lib/taski/execution/plain_progress_display.rb (2)

34-57: Consider extracting state-to-format mapping to reduce complexity.

RuboCop flags cyclomatic complexity (12/7). While the current implementation is readable, you could reduce complexity by extracting the format logic into a lookup structure. This is optional—the current code is clear and maintainable.

♻️ Optional refactor to reduce complexity

+      STATE_FORMATS = {
+        running: ->(name, _, _) { "[START] #{name}" },
+        completed: ->(name, dur, _) { "[DONE] #{name}#{dur ? " (#{dur})" : ""}" },
+        failed: ->(name, _, err) { "[FAIL] #{name}#{err ? ": #{err.message}" : ""}" },
+        cleaning: ->(name, _, _) { "[CLEAN] #{name}" },
+        clean_completed: ->(name, dur, _) { "[CLEAN DONE] #{name}#{dur ? " (#{dur})" : ""}" },
+        clean_failed: ->(name, _, err) { "[CLEAN FAIL] #{name}#{err ? ": #{err.message}" : ""}" }
+      }.freeze
+
       def on_task_updated(task_class, state, duration, error)
         return unless `@enabled`
 
-        case state
-        when :running
-          `@output.puts` "[START] #{short_name(task_class)}"
-        when :completed
-          duration_str = duration ? " (#{format_duration(duration)})" : ""
-          `@output.puts` "[DONE] #{short_name(task_class)}#{duration_str}"
-        when :failed
-          error_msg = error ? ": #{error.message}" : ""
-          `@output.puts` "[FAIL] #{short_name(task_class)}#{error_msg}"
-        when :cleaning
-          `@output.puts` "[CLEAN] #{short_name(task_class)}"
-        when :clean_completed
-          duration_str = duration ? " (#{format_duration(duration)})" : ""
-          `@output.puts` "[CLEAN DONE] #{short_name(task_class)}#{duration_str}"
-        when :clean_failed
-          error_msg = error ? ": #{error.message}" : ""
-          `@output.puts` "[CLEAN FAIL] #{short_name(task_class)}#{error_msg}"
+        formatter = STATE_FORMATS[state]
+        if formatter
+          formatted_duration = duration ? format_duration(duration) : nil
+          `@output.puts` formatter.call(short_name(task_class), formatted_duration, error)
         end
         `@output.flush`
       end

---

78-87: Inconsistent duration formatting in summary.

The summary uses raw milliseconds (#{total_duration}ms) while on_task_updated uses format_duration which converts to seconds for durations ≥1000ms. Consider using format_duration here for consistency:

-        `@output.puts` "[TASKI] Failed: #{failed}/#{total} tasks (#{total_duration}ms)"
+        `@output.puts` "[TASKI] Failed: #{failed}/#{total} tasks (#{format_duration(total_duration)})"
       else
-        `@output.puts` "[TASKI] Completed: #{completed}/#{total} tasks (#{total_duration}ms)"
+        `@output.puts` "[TASKI] Completed: #{completed}/#{total} tasks (#{format_duration(total_duration)})"

---

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 21ee56b and 0099dd7.

📒 Files selected for processing (1)

• lib/taski/execution/plain_progress_display.rb

🧰 Additional context used

🧬 Code graph analysis (1)

lib/taski/execution/plain_progress_display.rb (1)

lib/taski/execution/base_progress_display.rb (11)

• initialize (22-40)
• initialize (69-77)
• initialize (80-88)
• on_section_impl_registered (214-216)
• on_task_updated (224-226)
• state (43-45)
• duration (60-62)
• error (56-58)
• short_name (252-255)
• format_duration (258-264)
• should_activate? (235-237)

🪛 RuboCop (1.82.1)

lib/taski/execution/plain_progress_display.rb

[convention] 35-57: Cyclomatic complexity for on_task_updated is too high. [12/7]

(Metrics/CyclomaticComplexity)

🔇 Additional comments (5)

lib/taski/execution/plain_progress_display.rb (5)

5-16: Well-documented class with clear purpose.

The documentation clearly explains the use case (CI/non-TTY environments), output format examples, and how to enable the feature. Good addition to the codebase.

---

17-22: Good initialization with appropriate defaults.

Using $stderr as the default output is a sensible choice for CI environments—progress output won't interfere with actual program output on stdout. Setting sync = true ensures unbuffered output for real-time progress visibility.

---

59-62: LGTM!

Simple and correct—since @enabled is always true for this display, it will always activate when selected via TASKI_PROGRESS_MODE=plain.

---

64-72: LGTM!

Proper startup message with defensive check for @root_task_class. Consistent with the output format documented in the class header.

---

26-32: No thread safety issue exists here. The base class register_section_impl wraps the call to on_section_impl_registered inside @monitor.synchronize, so all @tasks access from this hook method is automatically protected by synchronization.

Likely an incorrect or invalid review comment.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[ahogappa]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.