feat(#558): Supervisor graph: epic node styling and priority arrow icons - #559
Merged
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds epic detection and serialization, excludes epics from ready tasks, auto-closes completed epics during supervisor polling, and renders epic and priority indicators in the dashboard. ChangesEpic hierarchy and auto-close
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@sova/dashboard/templates/supervisor.html`:
- Around line 840-853: The priority arrow paths created in the priority
rendering block lack a textual fallback. When appending each path for a
non-empty `priorityArrowPath`, add a `<title>` child containing the priority
name from `n.priority`, while preserving the existing shape, color, positioning,
and opacity.
In `@sova/supervisor/daemon.py`:
- Line 175: Update the _poll_epic_close method signature to annotate the adapter
parameter with the appropriate TaskAdapter type, matching the existing typing
convention used by TaskProgressionEngine. Preserve the method’s return
annotation and behavior.
In `@sova/supervisor/dependency_graph.py`:
- Around line 38-45: Rename the shared helper _is_epic to the public is_epic in
dependency_graph.py, then update all imports and call sites, including
progression.py and tests/test_epic_visual_hierarchy.py, to use the new symbol.
Preserve its current case-insensitive label matching behavior.
In `@sova/supervisor/progression.py`:
- Around line 379-434: Update auto_close_epics in sova/supervisor/progression.py
(lines 379-434) to obtain epic children through a dependency-graph/query path
that retains dependents even when DONE children lack milestones, rather than
relying on the schedule-filtered graph; preserve the no-children and all-DONE
checks. Add coverage in tests/test_epic_visual_hierarchy.py (lines 135-250)
using the real build_dependency_graph() with DONE, unmilestoned children and
verify the epic is auto-closed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: bc524712-741e-4693-95c4-c5a3fcd2b9f1
⛔ Files ignored due to path filters (1)
.claude/rules/architecture.mdis excluded by!.claude/**and included by none
📒 Files selected for processing (5)
sova/dashboard/templates/supervisor.htmlsova/supervisor/daemon.pysova/supervisor/dependency_graph.pysova/supervisor/progression.pytests/test_epic_visual_hierarchy.py
xsovad06
commented
Jul 30, 2026
xsovad06
left a comment
xsovad06
left a comment
Owner
Author
There was a problem hiding this comment.
Review: BLOCK
Critical bug: epic auto-close fails silently when DONE children lack milestones due to graph filtering—epics become stuck open forever. Missing integration test for this scenario. Also found: missing type hint on adapter param, fragile magic number for arrow positioning, and accessibility gap (no screen reader labels on priority arrows).
6 findings (all to be addressed)
- [CRITICAL] [bug]
sova/supervisor/progression.py:379: Epic auto-close silently fails for unmilestoned DONE children. If build_dependency_graph() filters DONE tasks with no milestone (as CodeRabbit flagged), then when all of an epic's children complete without milestones, get_dependents() returns empty set and the epic is skipped forever at line 407 (if not children: continue). This violates the spec requirement that epics should close when all children are DONE, leaving epics stuck open. Fix: Either: (1) Pass include_done=True explicitly even if it's the default to document intent, OR (2) Query adapter.list_tasks() directly to find all issues listing this epic in Dependencies (bypassing graph filtering), OR (3) Add milestone=None explicitly to prevent milestone-based filtering when building the graph for epic closure checks. Add integration test for unmilestoned DONE children scenario. - [HIGH] [testing]
tests/test_epic_visual_hierarchy.py:1: Test suite has no integration test for the milestone filtering scenario flagged by CodeRabbit. All auto_close_epics tests mock build_dependency_graph() to return a DependencyGraph directly (line 162), bypassing the actual graph building logic. If the graph filters DONE unmilestoned tasks, this critical bug won't be caught. Fix: Add integration test that: (1) creates epic with unmilestoned children via real adapter, (2) marks children DONE, (3) calls auto_close_epics without mocking build_dependency_graph, (4) asserts epic transitions to DONE. Use a real DependencyGraph built from adapter.list_tasks(). - [MEDIUM] [design]
sova/dashboard/templates/supervisor.html:830: Arrow offset for epic nodes uses magic number (40) that is fragile. Comment documents calculation (badge ends at 34, plus 6px margin), but if EPIC_BADGE_WIDTH or epicBadgeX constants change, ARROW_OFFSET_EPIC needs manual recalculation. This creates maintenance burden and risk of visual overlap bugs. Fix: Compute offset from badge constants:const ARROW_OFFSET_EPIC = 4 + EPIC_BADGE_WIDTH + 6; // epicBadgeX + width + margin. This makes the relationship explicit and updates automatically when badge dimensions change. - [MEDIUM] [api]
sova/supervisor/daemon.py:175: Missing type hint onadapterparameter. Project guidelines require 'All Python function signatures must include type hints.' The adapter param should be typed as TaskAdapter to match the pattern in TaskProgressionEngine constructor. Fix: Change signature to:async def _poll_epic_close(self, adapter: TaskAdapter) -> dict: - [MEDIUM] [docs]
sova/dashboard/templates/supervisor.html:844: Priority arrow SVG paths lack accessibility labels. Each element should have a <title> child for screen readers, similar to how the EPIC badge has text content. Users relying on assistive technology cannot distinguish priority levels from shape alone. Fix: Wrap each arrow path in a group with a title:const g = parentG.append('g'); g.append('title').text(n.priority.charAt(0).toUpperCase() + n.priority.slice(1) + ' priority'); g.append('path').attr('d', arrowPath)...This gives screen readers textual fallback. - [LOW] [design]
sova/supervisor/dependency_graph.py:38: _is_epic() has leading underscore but is imported across module boundaries (sova/supervisor/progression.py line 390, tests/test_epic_visual_hierarchy.py). Leading underscore signals 'internal to this module', but it's now de-facto public API. This violates Python convention and creates confusion about the function's intended scope. Fix: Rename to publicis_epic()(remove underscore) to reflect actual usage, or relocate to a shared utility module like sova/supervisor/utils.py if it's meant to be shared supervisor-wide.
xsovad06
dismissed
coderabbitai[bot]’s stale review
July 30, 2026 14:08
Findings addressed in latest push.
Add comprehensive epic tracking container support: - is_epic() helper function to detect type: epic label - Exclude epics from ready task list (tracking containers, not workable items) - Extract _are_dependencies_satisfied() for cleaner dependency checking - Preserve DONE tasks with dependencies in graph (ensures epic children stay visible even without milestones) - Auto-close epics when all child issues reach DONE state - Integrate epic auto-close into supervisor daemon poll cycle Epics are multi-issue tracking containers that group related work. They should not be assigned to agents but provide visual organization in the dependency graph and auto-close when their children complete.
Add visual distinction for epic nodes in the dependency graph: - Epic badge at bottom-left (mauve "EPIC" label) for tracking containers - Priority arrow icons with tooltips showing priority level - Distinct epic node styling with badge positioning calculations - Accessibility: arrow shape + color for priority indication Epic nodes now clearly stand out as tracking containers while priority arrows help users quickly identify critical/high priority items in the graph.
Add test coverage for epic tracking container functionality: - is_epic() label detection (case-insensitive, strict matching) - Epic exclusion from ready tasks (ensures tracking containers not assigned) - DONE task preservation (epic children stay in graph without milestones) - Epic child detection (get_dependents returns children correctly) - Auto-close success and failure paths - Graph build failure handling (fail-open behavior) - Unmilestoned epic children preservation - State transition failure paths Full coverage across dependency graph, progression engine, and daemon integration for the epic tracking container feature.
|
This was referenced Jul 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Changes
Frontend (supervisor.html)
PRIORITY_CONFIGwith SVG arrow paths for each priority level (critical: up + exclamation, high: up, medium: right, low: down)stroke-dasharray: '4,3') and 1.5px border widthBackend (supervisor/)
dependency_graph.py: Addedis_epic()helper andis_epicfield to graph nodesprogression.py: Newauto_close_epics()method checks all open epics and closes those with 100% children donedaemon.py: Auto-close check runs after each poll cycle (non-fatal side effect)get_ready_tasks()(treated as tracking containers, not actionable work)Tests
test_epic_visual_hierarchy.py) covering epic detection, auto-close logic, edge cases (no children, mixed states, circular dependencies)Documentation
Review guidance
Gap from issue scope: The issue requested only visual changes (epic styling + priority arrows). This PR also implements epic auto-close logic which was not requested. The auto-close behavior closes epics when all child issues are done, preventing stale epic containers. Review whether this should be:
supervisor.auto_close_epics)Arrow positioning: Epic nodes offset the arrow 40px from left (clear of epic badge which ends at 34px); regular nodes use 5px. This creates visual misalignment in mixed graphs but prevents overlap.
Epic detection: Uses
type: epiclabel only (same as existing exclusion logic). No validation that epics have children or that children list the epic correctly.Test plan
make serve, navigate to/supervisor, verify:Closes #558