fix(git): recover deleted symbols from the base revision (supersedes #72)#79
Conversation
`git diff --unified=0` emits a `+Z,0` hunk when lines are removed. These
deletion-only hunks contributed nothing to `changed_lines`, so downstream
symbol extraction in `core.rs` found no traceable symbol and skipped the
reference-traversal cascade entirely ("No traceable symbols found ...
skipping reference traversal"). The file's owning package was still marked
affected, but every *dependent* of the deleted symbol was missed.
Impact: any change whose diff is purely a deletion — removing a property
from an exported object, a case from a switch, an enum member, an overload —
under-reports affected projects to just the package the file lives in. In a
real monorepo, deleting one entry from an exported lookup object that 26
projects transitively consume reported `["@lama/selectors"]` (1 project)
instead of the 22 projects that actually reference it.
Fix: a `W == 0` deletion hunk now anchors to its new-side line `Z` — the
line just before the deletion point, which is still inside the enclosing
top-level symbol — so the AST lookup resolves that symbol and traces its
references normally. Clamped to 1 for deletions at the top of a file
(`+0,0`).
Known limitation (left for a follow-up): deleting an *entire* top-level
symbol leaves no enclosing node at the anchor line, so the AST lookup finds
nothing and behavior falls back to today's package-level marking. Catching
that requires parsing the base revision at the old-side range.
Adds a regression test and corrects two existing tests that had encoded the
buggy "deletion contributes nothing" assumption.
Reworks the deletion-only fix in response to review. The previous commit anchored a `+Z,0` deletion hunk to its new-side line `Z` and fed that into the current-file AST lookup. That heuristic is incorrect: after a deletion, line `Z` belongs to whatever symbol now occupies it in the *new* file, so deleting an entire top-level symbol either mis-attributes the change to the next symbol or resolves nothing — the deleted symbol's real dependents were still dropped. Instead, capture the hunk's *old-side* range `-X,Y` in a new `ChangedFile.deleted_lines` field and, downstream, re-parse the file at the base revision (`git show <merge-base>:./path`) to resolve the top-level symbol that enclosed each removed line. This handles both shapes uniformly: - removing a member of a still-present declaration (object property, switch case, interface field) resolves to the enclosing symbol, and - removing an entire exported declaration resolves to that symbol itself — the case the anchor heuristic could not solve. The recovered symbols are traced through the *current* import graph, where consumers still import them, so their projects are correctly reported affected. Deletions whose lines resolve to no declaration contribute nothing (no regression). Also drops the now-unreachable `has_hunks` empty-`changed_lines` branch that review flagged: every non-deletion hunk yields new-side lines and every deletion yields `deleted_lines`, so a file with hunks is never empty on both. analyzer: split the top-level-symbol resolution out of `find_node_at_line` into `find_top_level_symbols(&FileSemanticData, ...)` so it can run against a one-off base-revision parse (`parse_source`) that never enters `self.files`; profiler timing now wraps the call in `find_node_at_line`. Known limitation (unchanged from before): a whole-file deletion, and a deleted symbol reachable only via a local reference from another symbol whose binding the deletion breaks, still fall back to package-level marking. Tests: rewrite the three git unit tests for `deleted_lines` semantics, add a leading-symbol-deletion case, add analyzer unit tests for `find_deleted_symbols`, and add integration tests for whole-symbol and member deletion tracing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 2 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR fixes affected-project detection for pure-deletion diff hunks. ChangesDeletion-only change tracing
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Core as core.rs
participant Git as get_file_at_revision
participant Analyzer as find_deleted_symbols
participant Report
Core->>Git: fetch file content at merge-base revision
Git-->>Core: base file contents (or None)
Core->>Analyzer: resolve deleted_lines to enclosing symbols
Analyzer-->>Core: recovered deleted symbols
Core->>Report: record direct change cause (line: 0)
Core->>Core: extend unique_symbols with deleted_symbols
Core->>Core: run reference traversal across affected projects
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/integration_test.rs (1)
3946-3951: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider asserting
libis also affected for consistency with the first test.The first deletion test asserts both
libandappare affected, but this test only checksapp. Sincelib's source file was modified (thebetamember was removed),libshould also be marked affected per the PR objectives: "The affected file's package should still be marked." Adding this assertion would verify the direct-change package is included alongside the dependent.♻️ Suggested addition
assert!( affected.contains(&"app".to_string()), "app should be affected: `beta` was removed from `widget`, which app imports. Got: {:?}", affected ); + assert!( + affected.contains(&"lib".to_string()), + "lib should be affected (its file changed). Got: {:?}", + affected + );🤖 Prompt for 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. In `@tests/integration_test.rs` around lines 3946 - 3951, The deletion test is only checking the dependent package and misses the directly changed package, so update the affected-package assertions in this integration test to include both `lib` and `app`. Use the existing affected-set checks around this scenario to add an assertion that `lib` is present alongside the current `app` assertion, keeping the behavior consistent with the first deletion test and the `affected` collection semantics.
🤖 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 `@src/git.rs`:
- Around line 11-17: The stale test comment near the symmetric-hunk coverage
still refers to a greedy `.*` in `LINE_RE`, but `LINE_RE` now uses explicit
capture groups in `src/git.rs` and no longer matches that description. Update
the comment in the symmetric-hunk test to describe the current `LINE_RE`
behavior accurately, so future changes to `line regex` and the related hunk
handling logic are not justified by outdated assumptions.
---
Nitpick comments:
In `@tests/integration_test.rs`:
- Around line 3946-3951: The deletion test is only checking the dependent
package and misses the directly changed package, so update the affected-package
assertions in this integration test to include both `lib` and `app`. Use the
existing affected-set checks around this scenario to add an assertion that `lib`
is present alongside the current `app` assertion, keeping the behavior
consistent with the first deletion test and the `affected` collection semantics.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9f044d57-38e6-4b52-b89d-bf03dc733893
📒 Files selected for processing (7)
src/core.rssrc/git.rssrc/lockfile.rssrc/named_inputs.rssrc/semantic/analyzer.rssrc/types.rstests/integration_test.rs
📦 Preview Release AvailableA preview release has been published for commit 20e1d9c. Installationnpm install https://github.com/frontops-dev/domino/releases/download/pr-79-20e1d9c/front-ops-domino-1.4.0.tgzRunning the previewnpx https://github.com/frontops-dev/domino/releases/download/pr-79-20e1d9c/front-ops-domino-1.4.0.tgz affectedDetails |
- git.rs: fix stale symmetric-hunk test comment that referenced the old greedy `.*` in `LINE_RE`; describe the current explicit capture groups. - integration: assert the directly-changed package (`lib`) is also affected in the member-deletion test, mirroring the whole-symbol-deletion test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
|
||
| /// A file with changed lines | ||
| #[derive(Debug, Clone)] | ||
| #[derive(Debug, Clone, Default)] |
There was a problem hiding this comment.
why Default? doesn't look used anywhere — every construction (src + tests) sets both fields explicitly. drop it?
There was a problem hiding this comment.
Good catch — dropped the Default derive in 20e1d9c. It was a leftover from an earlier draft; every construction sets both fields explicitly.
| .push(AffectCause::DirectChange { | ||
| file: file_path.clone(), | ||
| symbol: Some(symbol.clone()), | ||
| line: 0, |
There was a problem hiding this comment.
line: 0 renders as (line 0) in the html report (report.rs:1820). harmless but reads oddly — maybe show (deleted) instead of a fake line?
There was a problem hiding this comment.
Agreed. Fixed in 20e1d9c: a deletion cause (line: 0) now renders (deleted) instead of (line 0), and the symbol-less whole-file/asset line-0 case drops the locator entirely. Verified on a real report — 0 (line 0) strings remain; the deleted field shows as partner.ts (deleted) → Symbol.
|
re: the two out-of-scope items — both correctly scoped out, neither regresses today's behavior. but they're not equal weight: whole-file deletion — worth a tracked follow-up. deleting a whole module that other projects import is a common refactor, and the gap is real: once the file is gone from disk, local-reference-only consumption — fine to defer. narrow edge, and direct cross-file imports (the common case) are handled. 👍 |
- types.rs: drop the unused `Default` derive on `ChangedFile` — every construction (src + tests) sets both fields explicitly. - report.rs: deletion causes carry `line: 0` (no surviving new-side line); render that as "(deleted)" instead of the odd "(line 0)", and omit the locator entirely for the symbol-less whole-file/asset line-0 case. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
EladBezalel
left a comment
EladBezalel
left a comment
There was a problem hiding this comment.
lgtm — correct approach (base-revision recovery), addresses the earlier anchor + dead-branch feedback, and the change is purely additive per the validation. the two nits are non-blocking follow-ups. 👏
3 participants
Closes #74. Supersedes #72.
Summary
domino affectedsilently drops the dependents of a deleted symbol. With--unified=0(always used byget_diff), removing lines produces a@@ -X,Y +Z,0 @@hunk whose new-side count is0, so it contributed nothing tochanged_lines. Downstream, no symbol was resolved, reference traversal was skipped, and every project depending on the deleted symbol was dropped — only the file's own package stayed marked.What changed since the first review
The first revision anchored a
+Z,0deletion to its new-side lineZand looked that up in the current file. As raised in review, that heuristic is incorrect:Zbelongs to whatever symbol now occupies it in the new file — not the deleted one. Deleting an entire top-level symbol either mis-attributes the change to the next symbol or resolves nothing, so the deleted symbol's real dependents were still dropped.has_hunksbranch (flagged by @EladBezalel and CodeRabbit).This revision replaces the heuristic with the semantically correct approach — issue direction (2): recover the deleted symbols from the base revision.
How it works now
git.rs—LINE_REnow captures both sides of the hunk header. A pure-deletion hunk (W == 0) records its old-side linesX..X+Yin a newChangedFile.deleted_linesfield; all other hunks contribute new-side lines tochanged_linesas before. The deadhas_hunksbranch is removed. New helperget_file_at_revisionreadsgit show <merge-base>:./path.analyzer.rs— the top-level-symbol resolution is split out offind_node_at_lineintofind_top_level_symbols(&FileSemanticData, …)so it can run against a one-off base-revision parse (parse_source) that never entersself.files. Newfind_deleted_symbols(file, base_source, deleted_lines)parses the base snapshot and resolves the enclosing top-level symbol at each removed line (de-duplicated).core.rs— for each changed file withdeleted_lines, fetch the base content at the merge-base and merge the recovered symbols into the reference-traversal set. They're traced through the current import graph — consumers stillimport { Foo }even afterFoois deleted — so their projects are correctly reported.This handles both shapes uniformly:
switchcase, interface field)Validation
Two-package Rush fixture;
package-bconsumeslookup,User, andOBSOLETEfrompackage-a:[package-a]❌[package-a, package-b]✅[package-a]❌[package-a, package-b]✅[package-a]❌[package-a, package-b]✅[package-a][package-a]✅ (stays precise)cargo test --lib— 212 passed.cargo clippy --all-targets --all-featuresandcargo fmt --checkclean. New integration tests:test_deletion_of_whole_exported_symbol_affects_dependents,test_deletion_of_member_affects_dependents_via_enclosing_symbol.Real-world validation
Reproduced against a real large Rush monorepo on the exact commit that slipped through CI. The change removed one field from an exported schema object in a shared client package — a pure deletion hunk:
Diffing that commit against its parent (
--base <parent> --head <commit>):The 41 newly-detected projects are exactly the downstream services and apps that import the schema and would break when the field is removed — the set CI skipped. Debug log confirms the mechanism:
Out of scope / known limitations
Both are follow-ups; neither regresses today's behavior.