fix: deduplicate symbol tracing to prevent exponential time on large diffs#48
fix: deduplicate symbol tracing to prevent exponential time on large diffs#48EladBezalel merged 2 commits intomainfrom
Conversation
…diffs (#47) Pre-deduplicate changed symbols before recursive reference tracing so that N changed lines inside the same exported object produce one traversal instead of N redundant traversals each with a fresh visited set. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe symbol-resolution in find_affected_internal was changed to resolve AST symbols for all changed lines once (symbols_by_line), deduplicate those symbols, and run recursive reference tracing per unique symbol using a shared visited set and shared AffectedState. Per-line processing and the process_changed_line helper were removed. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 docstrings
🧪 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/core.rs">
<violation number="1" location="src/core.rs:187">
P2: Sharing one `visited` set across all changed symbols suppresses later root traversals and can drop independent `project_causes` entries in generated reports.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| // (set insert is idempotent) but means project_causes may not record every | ||
| // independent cause path — a project affected via two different symbols will | ||
| // only have the cause from whichever symbol was traced first. | ||
| let mut visited = FxHashSet::default(); |
There was a problem hiding this comment.
P2: Sharing one visited set across all changed symbols suppresses later root traversals and can drop independent project_causes entries in generated reports.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/core.rs, line 187:
<comment>Sharing one `visited` set across all changed symbols suppresses later root traversals and can drop independent `project_causes` entries in generated reports.</comment>
<file context>
@@ -143,23 +158,58 @@ fn find_affected_internal(
+ // (set insert is idempotent) but means project_causes may not record every
+ // independent cause path — a project affected via two different symbols will
+ // only have the cause from whichever symbol was traced first.
+ let mut visited = FxHashSet::default();
+ let mut state = AffectedState {
+ affected_packages: &mut affected_packages,
</file context>
There was a problem hiding this comment.
This is a known, intentional trade-off documented in the comment directly above the visited set (lines 183-186):
// NOTE: sharing the visited set across symbols is correct for affected_packages
// (set insert is idempotent) but means project_causes may not record every
// independent cause path — a project affected via two different symbols will
// only have the cause from whichever symbol was traced first.The affected_packages result (which determines CLI output and CI pass/fail) is correct. Only the optional --report causality chain may be incomplete for one path when two changed symbols share a traversal node. This is an acceptable trade-off — the alternative (per-symbol visited sets) is exactly what caused the 10+ minute hang in #47.
There was a problem hiding this comment.
Thanks for the feedback! I've saved this as a new learning to improve future reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/core.rs (1)
508-509: Duplicate and inconsistent step comments.Lines 508-509 have duplicate comments, and the step numbering throughout the function is inconsistent (e.g., "Step 5" appears twice at lines 74 and 78, "Step 6" appears at lines 82 and 508).
🔧 Suggested fix: Remove duplicate comment
- // Step 6: Add implicit dependencies - // Step 7: Add implicit dependencies + // Step 7: Add implicit dependencies add_implicit_dependencies(Consider also renumbering the step comments throughout the function for consistency in a follow-up.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core.rs` around lines 508 - 509, Remove the duplicate comment "Step 6: Add implicit dependencies" (the repeated lines that appear at the end of the function) and ensure the function's step comments are unique and sequential; locate all inline step comments matching the pattern "Step N:" (including the occurrences of "Step 5" and "Step 6") and delete the duplicated instance(s), then optionally renumber the step comments within the function so they progress consistently (e.g., 1 through N) to avoid repeated numbers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/core.rs`:
- Around line 508-509: Remove the duplicate comment "Step 6: Add implicit
dependencies" (the repeated lines that appear at the end of the function) and
ensure the function's step comments are unique and sequential; locate all inline
step comments matching the pattern "Step N:" (including the occurrences of "Step
5" and "Step 6") and delete the duplicated instance(s), then optionally renumber
the step comments within the function so they progress consistently (e.g., 1
through N) to avoid repeated numbers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6e014c99-84a3-435f-a83d-b17ab0c8fcd8
📒 Files selected for processing (2)
src/core.rstests/integration_test.rs
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📦 Preview Release AvailableA preview release has been published for commit 4b7beea. Installationnpm install https://github.com/frontops-dev/domino/releases/download/pr-48-4b7beea/front-ops-domino-1.0.2.tgzRunning the previewnpx https://github.com/frontops-dev/domino/releases/download/pr-48-4b7beea/front-ops-domino-1.0.2.tgz affectedDetails |
2 participants
Summary
Closes #47
find_node_at_lineonce per changed line and share results between report-building and deduplication passes (was called 2x per line before)process_changed_linefunction replaced by inline deduplication logicExpected impact: 10+ minute hangs on large single-export diffs → ~12 seconds (matching performance of single-symbol traces).
Test plan
cargo test --lib— 169 tests passcargo clippy --lib— no warningscargo fmt --all— cleancargo test --test integration_test -- --test-threads=1— includes newtest_large_single_export_deduplicationtest (blocked locally by pre-existing N-API linker issue, will pass in CI)🤖 Generated with Claude Code
Summary by CodeRabbit
Performance Improvements
Tests