Skip to content

fix: do not re-evaluate internal graph nodes if all its descendants a…#2499

Merged
Aenimus merged 2 commits intomainfrom
david/eng-8910-fix-internal-graph-incorrect-node-re-evaluation
Feb 12, 2026
Merged

fix: do not re-evaluate internal graph nodes if all its descendants a…#2499
Aenimus merged 2 commits intomainfrom
david/eng-8910-fix-internal-graph-incorrect-node-re-evaluation

Conversation

@Aenimus
Copy link
Copy Markdown
Member

@Aenimus Aenimus commented Feb 12, 2026

…re resolvable

Summary by CodeRabbit

  • Bug Fixes

    • Corrected error message formatting to remove extra spacing in edge name listings
  • Refactor

    • Enhanced path resolution efficiency with early termination for already-resolved edges and streamlined variable usage
  • Tests

    • Simplified test case by removing redundant error scenario

Checklist

@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 12, 2026

Codecov Report

❌ Patch coverage is 93.33333% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 41.99%. Comparing base (746b1e3) to head (f13a316).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
composition/src/errors/errors.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #2499       +/-   ##
===========================================
- Coverage   61.77%   41.99%   -19.79%     
===========================================
  Files         231     1000      +769     
  Lines       24126   138762   +114636     
  Branches        0     8008     +8008     
===========================================
+ Hits        14905    58270    +43365     
- Misses       7978    78867    +70889     
- Partials     1243     1625      +382     
Files with missing lines Coverage Δ
...bility-graph/walker/entity-walker/entity-walker.ts 95.14% <100.00%> (ø)
composition/src/errors/errors.ts 82.83% <0.00%> (ø)

... and 790 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Feb 12, 2026

Walkthrough

This PR refactors path resolution handling in the entity walker by introducing an edgeSelectionPath variable to consolidate path construction logic, adds an early short-circuit return when edge descendants are already resolved, and consolidates nested resolution checks. Additionally, an error message formatting adjustment and a test case cleanup are included.

Changes

Cohort / File(s) Summary
Error Message Formatting
composition/src/errors/errors.ts
Minor formatting adjustment removes extra space after newline in error message construction.
Entity Walker Path Resolution
composition/src/resolvability-graph/walker/entity-walker/entity-walker.ts
Introduces edgeSelectionPath variable for path resolution, adds early short-circuit return for already-resolved descendants, replaces direct string construction across multiple call sites, updates selectionPathByEntityNodeName key construction, and consolidates nested isResolved checks into combined condition.
Test Case Cleanup
composition/tests/v1/resolvability.test.ts
Removes unresolvable field data block (unresolvableFieldDataFour) and corresponding expected error, reducing reported errors from 4 to 3.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • #2460: Modifies the same entity-walker file to change selection path and descendant resolution handling patterns.
  • #2240: Updates resolvability-graph traversal logic and error formatting constants in the same code paths.
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly addresses the main purpose of the PR: preventing re-evaluation of internal graph nodes when descendants are already resolvable.

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

✨ Finishing touches
  • 📝 Generate docstrings

No actionable comments were generated in the recent review. 🎉

Tip

Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord.


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

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Feb 12, 2026

Router-nonroot image scan passed

✅ No security vulnerabilities found in image:

ghcr.io/wundergraph/cosmo/router:sha-d8892a1a3d7ee1cee12e2d720b0e03aadfea1907-nonroot

@Aenimus Aenimus merged commit 2fa2dfd into main Feb 12, 2026
36 checks passed
@Aenimus Aenimus deleted the david/eng-8910-fix-internal-graph-incorrect-node-re-evaluation branch February 12, 2026 12:25
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.

3 participants