Skip to content

fix: render multi-parent groups once in the dependency tree - #78

Merged
7 commits merged into
mainfrom
fix/dag-render-multi-parent
Sep 27, 2026
Merged

7 commits merged into
mainfrom
fix/dag-render-multi-parent

Conversation

@vitali87

Copy link
Copy Markdown
Owner

Problem

Both DAG renderers (_render_dag for the terminal, _render_dag_markdown for the PR body) recurse from each parent to its children, so a group with more than one parent is emitted once per parent — along with its entire subtree and, in the PR body, the <-- this PR marker. For a diamond pr-1 → {pr-2, pr-3} → pr-4 → pr-5 the "Merge in this order" block listed pr-4 and pr-5 twice and marked "this PR" twice, which reads as two different PRs.

Fix

A merge node is drawn in full exactly once, under its last-listed parent, annotated (also depends on: <other parents>); under every other parent it appears as a one-line <id>: <title> (see below) reference so the reader still sees the edge. _render_dag (rich tree) gets the same treatment.

pr-1: base
├── pr-2: left
│   └── pr-4: merge (see below)
└── pr-3: right
    └── pr-4: merge (also depends on: pr-2)  <-- this PR
        └── pr-5: after

Tests

Diamond with a node below the merge: marker appears once, full merge node once, (see below) once, descendant once — for both renderers. 447 tests pass, ruff clean.

@coderabbitai

coderabbitai Bot commented Aug 30, 2026 •

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 51 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: a0fdfeaf-027d-4957-8a60-4b8a6027da09

📥 Commits

Reviewing files that changed from the base of the PR and between 671a081 and f2cf954.

📒 Files selected for processing (2)
  • pr_split/cli.py
  • tests/test_cli_helpers.py

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

Comment @coderabbitai help to get the list of available commands.

_render_dag and _render_dag_markdown walked the DAG as a tree, so a
group with several parents (and its whole subtree, and its '<-- this
PR' marker) appeared once per parent. The merge node is now drawn in
full under its last-listed parent with '(also depends on: ...)' and
referenced as '(see below)' under the others.
@vitali87
vitali87 force-pushed the fix/dag-render-multi-parent branch from 282915e to b701c9f Compare August 30, 2026 13:06
The '(see below)' stub was placed under every parent except depends_on[-1],
but the DFS visits parents in group order, so the stub could appear after
the full node. Render in full under the last parent actually visited and
drop the unreachable 'rendered' guard.
A parent that is never visited (it depends on an unknown group) left its
child stubbed forever with no full rendering. Compute the pending set
from a root-first reachability walk.
Tracking 'done' parents was wrong in both directions (a mid-walk
ancestor is neither done nor pending). Count visits instead: every
reachable parent is drawn exactly once, so a child is stubbed until its
final visit and drawn in full there.
@vitali87

Copy link
Copy Markdown
Owner Author

Reworked after local review: the renderers now count distinct reachable parents per node and draw a multi-parent node in full on its last visit, stubbing it as (see below) on earlier ones. This fixes three cases the first version got wrong — stub appearing after the full node when traversal order differed from depends_on order, a node vanishing when a parent is mid-walk or unreachable (depends on an unknown id), and duplicate depends_on entries hiding the node. Verified against a 4000-DAG randomised oracle (shuffled order, unknown/unreachable/duplicated deps, every current_id): exactly one full render, every stub before it, marker once. 9 tests in TestRenderDagMultiParent, all failing on main.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015rnEEjXWDBdePRhKMmZr1w
@vitali87 vitali87 closed this pull request by merging all changes into main in 8ca9e3b Sep 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants