-
Notifications
You must be signed in to change notification settings - Fork 0
fix: improve progress display accuracy for section candidates #136
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Mark unselected section candidate subtrees as completed (skipped) while excluding tasks that are also dependencies of the selected impl - Prevent state regression from :completed to :running in progress display - Show pending tasks when no running tasks available - Don't show success icon while pending tasks remain - Move collect_all_dependencies to BaseProgressDisplay for reuse Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
WalkthroughAdds recursive dependency collection and a run-state-finalized guard to BaseProgressDisplay; enhances SimpleProgressDisplay to track per-section candidate subtrees and mark unselected candidates (and their descendant tasks) completed; adds a test verifying subtree skipping behavior. (49 words) Changes
Sequence DiagramsequenceDiagram
participant Client as Section Handler
participant SPD as SimpleProgressDisplay
participant BPD as BaseProgressDisplay
participant Graph as Task Dependency Graph
Client->>SPD: register_section_impl(selected_candidate)
SPD->>BPD: collect_all_dependencies(selected_candidate)
BPD->>Graph: traverse cached_dependencies recursively
Graph-->>BPD: return dependency Set
BPD-->>SPD: selected_deps
SPD->>SPD: for each unselected_candidate
SPD->>BPD: collect_all_dependencies(unselected_candidate)
BPD->>Graph: traverse cached_dependencies recursively
Graph-->>BPD: return dependency Set
BPD-->>SPD: unselected_deps
SPD->>SPD: mark_subtree_completed(unselected_subtree, exclude: selected_deps)
SPD-->>SPD: mark pending tasks in subtree as completed (skip excluded)
SPD->>SPD: update internal state & render status line (pending/running/failed)
SPD-->>Client: section impl registered, display/state updated
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@lib/taski/execution/simple_progress_display.rb`:
- Around line 204-212: The truncation logic uses current_tasks.first(3) before
checking size, so pending_tasks never gets a "..." even when more than 3 items;
change the flow to compute the full list (e.g., pending_list =
pending_tasks.keys.map { |t| short_name(t) } or use current_tasks as the
canonical list), then create the displayed slice (full_list.first(3).join(", "))
and append "..." when full_list.size > 3; update the code that builds task_names
(and any branch that sets current_tasks/pending_tasks) so the size check
compares against the original full list, not the already-truncated array.
| elsif pending_tasks.any? | ||
| # Show pending tasks when nothing is running (waiting for dependencies) | ||
| pending_tasks.keys.first(3).map { |t| short_name(t) } | ||
| else | ||
| [] | ||
| end | ||
|
|
||
| task_names = current_tasks.first(3).join(", ") | ||
| task_names += "..." if current_tasks.size > 3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent truncation prevents "..." suffix for pending tasks.
Line 206 truncates to 3 tasks early via first(3), so current_tasks.size will never exceed 3 for pending tasks. This means line 212's "..." suffix won't appear even when there are more than 3 pending tasks, unlike the behavior for running/cleaning tasks.
Proposed fix
elsif pending_tasks.any?
# Show pending tasks when nothing is running (waiting for dependencies)
- pending_tasks.keys.first(3).map { |t| short_name(t) }
+ pending_tasks.keys.map { |t| short_name(t) }
else
[]
end🤖 Prompt for AI Agents
In `@lib/taski/execution/simple_progress_display.rb` around lines 204 - 212, The
truncation logic uses current_tasks.first(3) before checking size, so
pending_tasks never gets a "..." even when more than 3 items; change the flow to
compute the full list (e.g., pending_list = pending_tasks.keys.map { |t|
short_name(t) } or use current_tasks as the canonical list), then create the
displayed slice (full_list.first(3).join(", ")) and append "..." when
full_list.size > 3; update the code that builds task_names (and any branch that
sets current_tasks/pending_tasks) so the size check compares against the
original full list, not the already-truncated array.
- Extract run_state_finalized? predicate in BaseProgressDisplay - Split build_status_line into focused helper methods - Extract mark_unselected_candidates_completed for section handling - Extract mark_task_as_skipped for single task state update - Use safe navigation in on_section_impl_registered Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary
:completedto:runningin progress displaycollect_all_dependenciestoBaseProgressDisplayfor reuseTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.