Skip to content

fix: skip tsconfig excludes for direct changes#66

Merged
nirsky merged 2 commits intomainfrom
fix/skip-excludes-for-direct-changes
Apr 23, 2026
Merged

fix: skip tsconfig excludes for direct changes#66
nirsky merged 2 commits intomainfrom
fix/skip-excludes-for-direct-changes

Conversation

@nirsky
Copy link
Copy Markdown
Collaborator

@nirsky nirsky commented Apr 23, 2026

Summary

  • A directly changed file always belongs to its project — tsconfig excludes define what TypeScript compiles, not what belongs to a project
  • Previously, changing a .spec.ts or .stories.tsx file produced "no affected projects" because tsconfig.app.json excludes them
  • Added get_owning_packages_by_path — same path-prefix lookup as get_package_names_by_path but without tsconfig exclude filtering
  • Used for all direct-change ownership lookups in Steps 6a (source files) and 6b (assets)
  • The existing filtered method is preserved for reference traversal where cascade through excluded files may be undesirable

Before

change di.spec.ts →
  File "...di.spec.ts" excluded by tsconfig for project 'browser-desktop-background'
  Affected projects: []

After

change di.spec.ts →
  File "...di.spec.ts" belongs to package 'browser-desktop-background'
  Affected projects: ["browser-desktop-background", "browser-desktop-extension"]

Test plan

  • New unit test test_get_owning_packages_skips_tsconfig_excludes — verifies both methods side-by-side
  • 192 unit tests pass, 9 integration tests pass
  • End-to-end on 1685-project monorepo: di.spec.ts change correctly marks browser-desktop-background affected

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Improved detection of which projects are affected by file changes so that files excluded by build configuration (e.g., excluded test/config patterns) still mark their owning project as affected.
  • Tests

    • Added integration tests validating that direct edits to excluded files correctly mark the owning project as affected.

A directly changed file always belongs to its project — tsconfig
excludes define what TypeScript compiles, not project ownership.
Previously, changing a spec or stories file produced "no affected
projects" because tsconfig.app.json excludes *.spec.ts.

Added `get_owning_packages_by_path` — a path-prefix lookup that skips
tsconfig exclude filtering. Used for all direct-change ownership in
Steps 6a and 6b. The existing filtered `get_package_names_by_path`
is preserved for reference traversal where cascade through excluded
files may be undesirable.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 23, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: fd7fc452-8fb7-4913-b45b-fd7ea180c749

📥 Commits

Reviewing files that changed from the base of the PR and between 48939f6 and 7c5e1c7.

📒 Files selected for processing (2)
  • src/utils.rs
  • tests/integration_test.rs

📝 Walkthrough

Walkthrough

Swaps the project-lookup used when mapping changed files from the tsconfig-filtered get_package_names_by_path to a new unfiltered get_owning_packages_by_path, and adds tests verifying ownership differs when tsconfig excludes would otherwise hide files.

Changes

Cohort / File(s) Summary
Core ownership swap
src/core.rs
Replaces calls to get_package_names_by_path with get_owning_packages_by_path for direct file ownership mapping; updates inline comments to note excludes are bypassed (+7/-5).
Package resolution & tests
src/utils.rs, tests/integration_test.rs
Extracts shared resolver into resolve_packages(..., skip_excludes), adds pub fn get_owning_packages_by_path(&self, file_path: &Path) -> Vec<String>, keeps get_package_names_by_path filtered, and adds integration/unit tests validating filtered vs unfiltered behavior (+129/-21; +75/-0).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • EladBezalel

Poem

🐰 I nibble through code paths, soft and spry,
Unhiding owners where hidden files lie.
A hop, a sniff, a mapping true—
Projects found where excludes once grew. ✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix: skip tsconfig excludes for direct changes' directly and concisely describes the main change: implementing logic to skip tsconfig excludes when determining ownership for directly modified files.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/skip-excludes-for-direct-changes

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 and usage tips.

Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 2 files

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 23, 2026

📦 Preview Release Available

A preview release has been published for commit 7c5e1c7.

Installation

npm install https://github.com/frontops-dev/domino/releases/download/pr-66-7c5e1c7/front-ops-domino-1.2.5.tgz

Running the preview

npx https://github.com/frontops-dev/domino/releases/download/pr-66-7c5e1c7/front-ops-domino-1.2.5.tgz affected

Details

Copy link
Copy Markdown
Collaborator

@EladBezalel EladBezalel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean separation of ownership vs reference traversal — exactly the right approach. Two suggestions, neither blocking.

Comment thread src/utils.rs Outdated
Comment thread src/utils.rs
…n tests

Address PR #66 review:

- Extract private `resolve_packages(file_path, skip_excludes)` shared by
  both public methods — prevents silent divergence if matching logic
  evolves.

- Add `test_get_owning_packages_fast_path_no_root_entries`: exercises the
  `root_entries.is_empty()` branch (root == sourceRoot + tsconfig
  excludes).

- Add `test_spec_file_change_affects_owning_project`: full pipeline
  integration test — creates a project with tsconfig excluding
  *.spec.ts, commits a spec change, asserts `find_affected` returns the
  owning project.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Collaborator

@EladBezalel EladBezalel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All review feedback addressed — helper extracted, fast-path tested, integration test added. LGTM.

@nirsky nirsky merged commit ab996c4 into main Apr 23, 2026
32 of 38 checks passed
@nirsky nirsky deleted the fix/skip-excludes-for-direct-changes branch April 23, 2026 11:42
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