return latest diff for merged branch#8485
Merged
ajtmccarty merged 5 commits intorelease-1.8from Feb 27, 2026
Merged
Conversation
Contributor
WalkthroughAdds a read-only 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
github-actions
Bot
added
the
group/backend
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
backend/infrahub/graphql/queries/diff/tree.py (1)
47-60: Prefer Pydantic models for query parameter containers.
DiffBranchInfoandDiffQueryParamsare new dataclasses; this file path is under backend Python rules that require Pydantic models for these structures.As per coding guidelines,
**/*.py: Use Pydantic models for dataclasses in Python.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/infrahub/graphql/queries/diff/tree.py` around lines 47 - 60, Replace the two dataclasses DiffBranchInfo and DiffQueryParams with Pydantic models by subclassing pydantic.BaseModel (keep the field names and types: name, branch_start_timestamp, is_terminal for DiffBranchInfo and from_time, to_time, tracking_id, exclude_merged for DiffQueryParams), import any needed types (Timestamp, TrackingId) and pydantic.Field if you need defaults/aliases, and ensure optional fields use typing.Optional or | None as currently used; update any code that constructs or validates these objects to use the Pydantic model constructors/attributes instead of dataclass behavior.backend/infrahub/core/diff/coordinator.py (1)
180-185: Return the newest stored diff explicitly.Using
diff_roots[0]relies on implicit query ordering. Selecting by latestto_timemakes the behavior deterministic.♻️ Proposed adjustment
diff_roots = await self.diff_repo.get_roots_metadata( diff_branch_names=[diff_branch.name], tracking_id=tracking_id, exclude_merged=False ) if not diff_roots: raise ResourceNotFoundError(f"No stored diff found for terminal branch {diff_branch.name}") - return diff_roots[0] + return max(diff_roots, key=lambda d: d.to_time.to_datetime())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/infrahub/core/diff/coordinator.py` around lines 180 - 185, The code currently returns diff_roots[0] which relies on implicit ordering; update the logic in the method that calls self.diff_repo.get_roots_metadata (in coordinator.py) to explicitly pick the newest stored diff by selecting the element with the maximal to_time (e.g., use max(diff_roots, key=lambda r: r.to_time) or equivalent) and return that entry, while keeping the ResourceNotFoundError branch intact when diff_roots is empty.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/infrahub/graphql/queries/diff/tree.py`:
- Around line 526-531: The code always sets exclude_merged based on
branch_info.is_terminal or not proposed_change_id, which causes merged diffs to
be excluded even when explicit time filters (from_timestamp or to_timestamp) are
provided; modify the DiffQueryParams construction so exclude_merged is False if
either from_timestamp or to_timestamp is set (i.e., only exclude merged when no
explicit time window and (branch_info.is_terminal or not proposed_change_id)),
keeping the existing symbols DiffQueryParams, branch_info.is_terminal,
proposed_change_id, from_timestamp, and to_timestamp to locate the change.
---
Nitpick comments:
In `@backend/infrahub/core/diff/coordinator.py`:
- Around line 180-185: The code currently returns diff_roots[0] which relies on
implicit ordering; update the logic in the method that calls
self.diff_repo.get_roots_metadata (in coordinator.py) to explicitly pick the
newest stored diff by selecting the element with the maximal to_time (e.g., use
max(diff_roots, key=lambda r: r.to_time) or equivalent) and return that entry,
while keeping the ResourceNotFoundError branch intact when diff_roots is empty.
In `@backend/infrahub/graphql/queries/diff/tree.py`:
- Around line 47-60: Replace the two dataclasses DiffBranchInfo and
DiffQueryParams with Pydantic models by subclassing pydantic.BaseModel (keep the
field names and types: name, branch_start_timestamp, is_terminal for
DiffBranchInfo and from_time, to_time, tracking_id, exclude_merged for
DiffQueryParams), import any needed types (Timestamp, TrackingId) and
pydantic.Field if you need defaults/aliases, and ensure optional fields use
typing.Optional or | None as currently used; update any code that constructs or
validates these objects to use the Pydantic model constructors/attributes
instead of dataclass behavior.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/infrahub/core/branch/models.pybackend/infrahub/core/diff/coordinator.pybackend/infrahub/graphql/queries/diff/tree.pybackend/tests/component/core/diff/test_coordinator.pybackend/tests/component/graphql/diff/test_diff_tree_terminal_branch.py
ogenstad
approved these changes
Feb 27, 2026
| branch: str | None, | ||
| proposed_change_id: str | None, | ||
| ) -> tuple[str, Timestamp | None]: | ||
| ) -> DiffBranchInfo: |
| diff_branch_name = branch # Use the requested branch name for the query | ||
| branch_start_timestamp = None | ||
| if not branch: | ||
| raise ValidationError("Must include the branch or proposed_change_id argument") from None |
Contributor
There was a problem hiding this comment.
The error message looks wrong here, as we raise earlier if we're missing proposed_change_id. Is something wrong with the code here or should it read "Must include the branch and proposed_change_id argument"?
Contributor
Author
There was a problem hiding this comment.
you are correct that this was wrong
it would have prevented retrieving a diff using a proposed change ID for a deleted branch
I've fixed it and added tests for it
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
backend/infrahub/graphql/queries/diff/tree.py (1)
526-531:⚠️ Potential issue | 🟠 MajorExplicit time filters should not force merged-diff exclusion.
With Line 530, terminal branches still get
exclude_merged=Truewhenfrom_time/to_timeare provided, so matching merged diffs are filtered out.Proposed fix
return DiffQueryParams( from_time=from_timestamp, to_time=to_timestamp, tracking_id=NameTrackingId(name) if name else None, - exclude_merged=branch_info.is_terminal or not proposed_change_id, + exclude_merged=(not from_time and not to_time) and (branch_info.is_terminal or not proposed_change_id), )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/infrahub/graphql/queries/diff/tree.py` around lines 526 - 531, The current DiffQueryParams construction forces exclude_merged=True when branch_info.is_terminal or not proposed_change_id, even if explicit time filters (from_timestamp/to_timestamp) are provided; change the exclude_merged logic so that explicit time filters disable that forced exclusion. Update the return that builds DiffQueryParams (the call creating DiffQueryParams with from_time=from_timestamp, to_time=to_timestamp, tracking_id=NameTrackingId(name) if name else None) to compute exclude_merged as ((branch_info.is_terminal or not proposed_change_id) and not (from_timestamp or to_timestamp)) so merged diffs are not excluded when a from_time or to_time is supplied.
🧹 Nitpick comments (1)
backend/infrahub/graphql/queries/diff/tree.py (1)
47-60: Replace new stdlib dataclasses with Pydantic models for consistency.These new transport structs are fine functionally, but they violate the repository rule to use Pydantic for dataclass-like models.
As per coding guidelines, "Use Pydantic models for dataclasses in Python".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/infrahub/graphql/queries/diff/tree.py` around lines 47 - 60, The two dataclasses DiffBranchInfo and DiffQueryParams should be converted to Pydantic models to follow repo conventions: replace `@dataclass` declarations with classes inheriting from pydantic.BaseModel, change nullable union types (Timestamp | None, TrackingId | None) to Optional[Timestamp]/Optional[TrackingId] (import Optional from typing), keep the same field names and types and the boolean field exclude_merged, and add any needed Config (e.g., orm_mode = True) if other code expects model-to-dict/ORM interoperability; update imports accordingly and ensure any code constructing these types uses the Pydantic constructors instead of dataclass semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/infrahub/graphql/queries/diff/tree.py`:
- Around line 493-498: The deleted-branch fallback currently runs whenever
branch is falsy, allowing silent empty lookups; update the conditional that
returns the DiffBranchInfo fallback to require a provided proposed_change_id as
well. In the block that now reads "if branch: return DiffBranchInfo(...)",
change it to check "if branch or proposed_change_id:" (so the fallback is only
used when a proposed_change_id was supplied) and keep the earlier
ValidationError check intact; reference DiffBranchInfo, branch, and
proposed_change_id to locate the change.
In `@backend/tests/component/graphql/diff/test_diff_tree_terminal_branch.py`:
- Around line 124-131: The TestDiffTreeTerminalBranch class is order-dependent
because the deleted_branch fixture mutates shared state other merged-branch
tests rely on; fix by extracting the deleted_branch fixture and all tests that
use it into a new separate test class (e.g., TestDiffTreeDeletedBranch) so they
run with isolated fixtures and no shared merged_branch usage; locate the
deleted_branch fixture and the tests that reference it and move their
definitions into the new class, ensuring imports/fixtures still resolve and
removing any implicit dependency on merged_branch in the new class.
---
Duplicate comments:
In `@backend/infrahub/graphql/queries/diff/tree.py`:
- Around line 526-531: The current DiffQueryParams construction forces
exclude_merged=True when branch_info.is_terminal or not proposed_change_id, even
if explicit time filters (from_timestamp/to_timestamp) are provided; change the
exclude_merged logic so that explicit time filters disable that forced
exclusion. Update the return that builds DiffQueryParams (the call creating
DiffQueryParams with from_time=from_timestamp, to_time=to_timestamp,
tracking_id=NameTrackingId(name) if name else None) to compute exclude_merged as
((branch_info.is_terminal or not proposed_change_id) and not (from_timestamp or
to_timestamp)) so merged diffs are not excluded when a from_time or to_time is
supplied.
---
Nitpick comments:
In `@backend/infrahub/graphql/queries/diff/tree.py`:
- Around line 47-60: The two dataclasses DiffBranchInfo and DiffQueryParams
should be converted to Pydantic models to follow repo conventions: replace
`@dataclass` declarations with classes inheriting from pydantic.BaseModel, change
nullable union types (Timestamp | None, TrackingId | None) to
Optional[Timestamp]/Optional[TrackingId] (import Optional from typing), keep the
same field names and types and the boolean field exclude_merged, and add any
needed Config (e.g., orm_mode = True) if other code expects model-to-dict/ORM
interoperability; update imports accordingly and ensure any code constructing
these types uses the Pydantic constructors instead of dataclass semantics.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/infrahub/graphql/queries/diff/tree.pybackend/tests/component/graphql/diff/test_diff_tree_terminal_branch.py
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
backend/infrahub/graphql/queries/diff/tree.py (1)
526-531:⚠️ Potential issue | 🟠 MajorMerged branches are still excluded when explicit time filters are provided.
At Line 530,
exclude_merged=branch_info.is_terminal or not proposed_change_idstill forces exclusion for terminal branches even whenfrom_time/to_timeare explicitly set.Proposed fix
return DiffQueryParams( from_time=from_timestamp, to_time=to_timestamp, tracking_id=NameTrackingId(name) if name else None, - exclude_merged=branch_info.is_terminal or not proposed_change_id, + exclude_merged=(not branch_info.is_terminal) and (not proposed_change_id), )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/infrahub/graphql/queries/diff/tree.py` around lines 526 - 531, The current DiffQueryParams creation forces exclude_merged when branch_info.is_terminal is true (or when proposed_change_id is falsy) even if explicit time filters are provided; change the exclude_merged expression in the DiffQueryParams call so merged branches are not automatically excluded when from_timestamp or to_timestamp are set—e.g. compute exclude_merged = ((branch_info.is_terminal or not proposed_change_id) and not (from_timestamp or to_timestamp)) and pass that into DiffQueryParams (keep using NameTrackingId(name) for tracking_id).
🧹 Nitpick comments (1)
backend/tests/component/graphql/diff/test_diff_tree_terminal_branch.py (1)
240-267: Add a positive explicit-time-window assertion for merged branches.This test only checks a non-matching future window. Please add one matching-range case to ensure explicit filters can still return merged diffs when they should.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/component/graphql/diff/test_diff_tree_terminal_branch.py` around lines 240 - 267, The test test_diff_tree_merged_branch_honors_explicit_time_filters only asserts a non-matching future time window; add a second positive case that uses a time window that should match merged diffs to verify explicit filters can return results. Update the test to call graphql again with DIFF_TREE_QUERY_WITH_TIME_FILTERS using merged_branch.name and a from_time/to_time range that surrounds the merge timestamps (e.g., a window covering the branch's known commit/merge time), then assert result.errors is None, result.data is truthy, and result.data["DiffTree"] is not None (or contains expected nodes) to confirm merged diffs are returned. Ensure the new assertions reference the same params/context prepared by prepare_graphql_params and reuse merged_branch to locate the correct series.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@backend/infrahub/graphql/queries/diff/tree.py`:
- Around line 526-531: The current DiffQueryParams creation forces
exclude_merged when branch_info.is_terminal is true (or when proposed_change_id
is falsy) even if explicit time filters are provided; change the exclude_merged
expression in the DiffQueryParams call so merged branches are not automatically
excluded when from_timestamp or to_timestamp are set—e.g. compute exclude_merged
= ((branch_info.is_terminal or not proposed_change_id) and not (from_timestamp
or to_timestamp)) and pass that into DiffQueryParams (keep using
NameTrackingId(name) for tracking_id).
---
Nitpick comments:
In `@backend/tests/component/graphql/diff/test_diff_tree_terminal_branch.py`:
- Around line 240-267: The test
test_diff_tree_merged_branch_honors_explicit_time_filters only asserts a
non-matching future time window; add a second positive case that uses a time
window that should match merged diffs to verify explicit filters can return
results. Update the test to call graphql again with
DIFF_TREE_QUERY_WITH_TIME_FILTERS using merged_branch.name and a
from_time/to_time range that surrounds the merge timestamps (e.g., a window
covering the branch's known commit/merge time), then assert result.errors is
None, result.data is truthy, and result.data["DiffTree"] is not None (or
contains expected nodes) to confirm merged diffs are returned. Ensure the new
assertions reference the same params/context prepared by prepare_graphql_params
and reuse merged_branch to locate the correct series.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/infrahub/graphql/queries/diff/tree.pybackend/tests/component/graphql/diff/test_diff_tree_terminal_branch.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TODO:
Problem
when a user requests a diff via graphql using the
DiffTreeor `DiffTreeSummary by CodeRabbit
-
- For branches in a terminal state (merged or deleting), diff endpoints now return the most recent stored result instead of recalculating, improving consistency and performance.
- Branch objects expose a read-only terminal-state check to power this behavior.
- Diff query handling and time/filters were refined to better support terminal branches and proposed-change lookups.
-
- Added comprehensive tests covering diff queries, summaries, updates, time-filtering, and terminal-branch behaviors.
` queries, if the branch in question has been merged, the `DiffTree` queries will try to retrieve the latest diff for the branch using the branch's `branched_from` time. `branched_from` is updated to the time of the merge when a branch is merged and this causes the diff to appear empty for a merged branch.Improvements
Tests
Fixes
DiffTreeresolvers