Fix tree rendering of shared dependencies (diamond graphs)#237
Conversation
|
Warning Review limit reached
More reviews will be available in 51 minutes and 40 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR refactors how tree-line prefixes are managed: instead of computing them from task-class state on-the-fly, prefixes are now computed once during node registration and stored directly on each node. The prefix API is split into a lookup function and a new helper for computing prefixes from traversal state. Tests validate correct rendering of shared task occurrences with their position-specific prefixes. ChangesTree Prefix Storage and Computation Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
3968cf7 to
3a809d8
Compare
A shared dependency (Root -> Left -> Shared, Root -> Shared) is
registered once per occurrence when the dependency DAG is expanded into
a tree, but register_tree_nodes keyed depth/is_last/ancestors_last by
TASK CLASS — so the last occurrence clobbered every earlier one and all
rows for that class rendered with the same (wrong) prefix:
Root Root
├── Left ├── Left
└── Shared instead of │ └── Shared
└── Shared └── Shared
The prefix belongs to the occurrence: it is now computed at registration
and stored on the tree node itself; build_node_lines reads it from the
node. The class-keyed map keeps only the FIRST occurrence, as the one
canonical prefix for Tree::Event's per-event output lines (a shared
task runs once, so its event line prints at the position it is first
shown in the tree).
The same first-wins change to @tree_nodes fixes a second, more severe
class-keying bug: when the ROOT class reappears as a circular leaf
(Root -> A -> Root), master clobbered the root's node with the
childless leaf, so build_tree_lines walked the leaf and Task.tree
collapsed to a single orphan line (" └── Root"). Both behaviors are
pinned: an exact full-tree diamond assertion, an event-line assertion,
and an exact full-tree root-cycle assertion.
Affects render_tree (Task.tree, Tree::Live live/final frames) and
Tree::Event event lines.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
3a809d8 to
b714bd0
Compare
1 participant
Problem
Found while recording the theme demo for #235/#236 — pre-existing on master (verified byte-identical there). When a task is a shared dependency (diamond:
Root → Left → SharedandRoot → Shared), the rendered tree duplicates the row with a broken prefix:register_tree_nodeskeyeddepth/is_last/ancestors_lastby task class, so the last-registered occurrence clobbered every earlier one — all rows for that class rendered with the same prefix.Fix
The prefix belongs to the occurrence, not the class: it is computed at registration and stored on the tree node;
build_node_linesreads it from the node. The class-keyed map keeps only the first occurrence's prefix as the one canonical prefix forTree::Event's per-event lines (a shared task runs once; its event line prints at the position it first appears in the tree).Second bug fixed by the same first-wins change (found by the adversarial review): when the root class reappears as a circular leaf (
Root → A → Root—Task.treerenders cyclic graphs by design, cycle validation only runs at execution), master clobbered the root's node with the childless circular leaf, sobuild_tree_lineswalked the leaf and the whole tree collapsed to a single orphan line (└── Root). Now renders the full tree with the cycle as a leaf occurrence.Affects
render_tree(Task.tree,Tree::Livelive/final frames) andTree::Eventevent lines.Tests
||=→=revert mutant, which previously survived the full suite)rake standardcleanAdversarial review
28-agent review across the three follow-up PRs: for this PR, deep diamonds / multiple shared nodes / shared subtrees / circular markers / Live lifecycle all verified correct, non-shared output byte-identical to master. 1 confirmed finding (the untested root-cycle behavior above) — folded in.
🤖 Generated with Claude Code