Conversation
urangel
added
user interface
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughIntroduces direct-vs-indirect descendant routing for Azure graph traversal: ChangesAzure Descendant Traversal Refactoring
EntityInfoDataTableGraphed Count Normalization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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: 2
🧹 Nitpick comments (2)
packages/javascript/bh-shared-ui/src/components/EntityInfoDataTableGraphed/EntityInfoDataTableGraphed.tsx (1)
26-26: 💤 Low valueType annotation does not match
Promise.allSettledoutput.
Promise.allSettledreturnsPromiseSettledResult<T>[](union ofPromiseFulfilledResultandPromiseRejectedResult), but the parameter is typed asPromiseFulfilledResult<T>[]. The runtime filtering on line 30 handles both cases, but the type is inaccurate.♻️ Suggested type correction
function getCount<T>( - queryData: Array<PromiseFulfilledResult<PaginatedResponse<T>>> | PaginatedResponse<T> | undefined, + queryData: Array<PromiseSettledResult<PaginatedResponse<T>>> | PaginatedResponse<T> | undefined, countLabel: string | undefined ): number | undefined {🤖 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 `@packages/javascript/bh-shared-ui/src/components/EntityInfoDataTableGraphed/EntityInfoDataTableGraphed.tsx` at line 26, The type annotation for the queryData parameter on line 26 in EntityInfoDataTableGraphed.tsx does not accurately reflect the output of Promise.allSettled, which returns a union of both PromiseFulfilledResult and PromiseRejectedResult. Change the type annotation for queryData from Array<PromiseFulfilledResult<PaginatedResponse<T>>> to Array<PromiseSettledResult<PaginatedResponse<T>>> to correctly represent the complete settled result type that includes both fulfilled and rejected promises.packages/go/analysis/azure/azure_integration_test.go (1)
953-1046: ⚡ Quick winAdd a sibling-branch regression test for non-tenant roots.
These new tests only anchor the root at a tenant. Please add a case with a non-tenant root (e.g., one subscription under a tenant with a sibling subscription) and assert only the selected root’s subtree is returned.
🤖 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 `@packages/go/analysis/azure/azure_integration_test.go` around lines 953 - 1046, Add a new test function after TestFetchEntityDescendentPaths_NoTerminalsReturnsEmpty to verify that FetchEntityDescendentPaths correctly handles non-tenant root nodes. The test should create a tenant with two sibling subscriptions, each containing an application, then call FetchEntityDescendentPaths with one subscription as the root and graphAzure.App as the target kind. Assert that the returned paths contain only the nodes from the selected root's subtree (the chosen subscription and its app), excluding the sibling subscription and its app. This ensures the function properly isolates each branch when starting from a non-tenant node.
🤖 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 `@packages/go/analysis/azure/queries.go`:
- Around line 860-870: The FetchDescendentKindByTenantID function at lines
860-870 filters nodes only by tenant ID and kind, but does not ensure the
results are actual descendants of the provided root node. This allows sibling
branches in the same tenant to be included in results. Additionally, the code at
lines 897-915 applies this same flawed traversal logic. Fix this by adding a
constraint to the query that ensures returned nodes are actual descendants of
the root node parameter, not merely nodes within the same tenant. This
constraint should be applied in the query.And() call at line 862 where you build
the filter conditions, and any similar filtering logic at lines 897-915 should
also be updated to include this descendant relationship validation to properly
scope results to the requested root's subtree.
In
`@packages/javascript/bh-shared-ui/src/components/EntityInfoDataTableGraphed/EntityInfoDataTableGraphed.tsx`:
- Around line 32-35: The `forEach` loop with a `return` statement inside the
callback only exits the callback, not the outer `getCount` function, causing the
function to fall through and return `undefined` implicitly. Replace the
`forEach` call with the `find` method when filtering `fulfilledData` by
`countLabel`. The `find` method will properly return the matching `sectionData`
object, allowing you to extract and return its `count` property directly, fixing
the countLabel filtering feature so the correct count is displayed instead of
`undefined`.
---
Nitpick comments:
In `@packages/go/analysis/azure/azure_integration_test.go`:
- Around line 953-1046: Add a new test function after
TestFetchEntityDescendentPaths_NoTerminalsReturnsEmpty to verify that
FetchEntityDescendentPaths correctly handles non-tenant root nodes. The test
should create a tenant with two sibling subscriptions, each containing an
application, then call FetchEntityDescendentPaths with one subscription as the
root and graphAzure.App as the target kind. Assert that the returned paths
contain only the nodes from the selected root's subtree (the chosen subscription
and its app), excluding the sibling subscription and its app. This ensures the
function properly isolates each branch when starting from a non-tenant node.
In
`@packages/javascript/bh-shared-ui/src/components/EntityInfoDataTableGraphed/EntityInfoDataTableGraphed.tsx`:
- Line 26: The type annotation for the queryData parameter on line 26 in
EntityInfoDataTableGraphed.tsx does not accurately reflect the output of
Promise.allSettled, which returns a union of both PromiseFulfilledResult and
PromiseRejectedResult. Change the type annotation for queryData from
Array<PromiseFulfilledResult<PaginatedResponse<T>>> to
Array<PromiseSettledResult<PaginatedResponse<T>>> to correctly represent the
complete settled result type that includes both fulfilled and rejected promises.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: b184f80c-bbbe-467e-ba8d-6a6f7613c556
📒 Files selected for processing (9)
cmd/api/src/api/v2/azure.gopackages/go/analysis/azure/azure_integration_test.gopackages/go/analysis/azure/db_ops.gopackages/go/analysis/azure/management_group.gopackages/go/analysis/azure/queries.gopackages/go/analysis/azure/resource_group.gopackages/go/analysis/azure/subscription.gopackages/go/analysis/azure/tenant.gopackages/javascript/bh-shared-ui/src/components/EntityInfoDataTableGraphed/EntityInfoDataTableGraphed.tsx
coderabbitai
Bot
added
api
AD7ZJ
reviewed
Jun 23, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/go/analysis/azure/azure_integration_test.go (1)
1-1122: 📐 Maintainability & Code Quality | 🟡 MinorUpdate license header copyright year to 2026.
The file has the correct license header structure and build tag (
//go:build integration) placed correctly underneath it (line 17). However, the copyright year in the license header (line 1) is 2023, while theLICENSE.headerreference file specifies 2026. Per the coding guidelines, the license header must use the up-to-date version fromLICENSE.header.Update copyright year from 2023 to 2026:
-// Copyright 2023 Specter Ops, Inc. +// Copyright 2026 Specter Ops, Inc.🤖 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 `@packages/go/analysis/azure/azure_integration_test.go` around lines 1 - 1122, The copyright year in the license header at the beginning of the file is outdated and needs to be updated from 2023 to 2026 to match the current LICENSE.header reference file and coding guidelines. Update the copyright year in the first line of the license header comment block that appears before the build tag.Source: Coding guidelines
🤖 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 `@packages/go/analysis/azure/azure_integration_test.go`:
- Around line 1124-1170: The test
TestListEntityDescendents_NestedManagementGroupToSubscription expects nestedSub
to be returned as a descendent of mgRoot through a 2-hop path (mgRoot → mgChild
→ nestedSub), but ListEntityDescendents routes to FetchDirectEntityDescendents
which only performs single-hop filtering on direct relationships. To fix this
mismatch, either remove the intermediate mgChildNode and create a direct
Contains relationship from mgRootNode to nestedSubNode to match the single-hop
behavior of FetchDirectEntityDescendents, or if multi-hop traversal is required,
modify the implementation to use FetchEntityDescendents instead of
FetchDirectEntityDescendents for this scenario. Choose the approach that aligns
with the intended Azure resource hierarchy behavior.
---
Outside diff comments:
In `@packages/go/analysis/azure/azure_integration_test.go`:
- Around line 1-1122: The copyright year in the license header at the beginning
of the file is outdated and needs to be updated from 2023 to 2026 to match the
current LICENSE.header reference file and coding guidelines. Update the
copyright year in the first line of the license header comment block that
appears before the build tag.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: d629cc23-8aac-4b3f-9015-2751e77bd2d5
📒 Files selected for processing (1)
packages/go/analysis/azure/azure_integration_test.go
AD7ZJ
approved these changes
Jun 23, 2026
AD7ZJ
left a comment
AD7ZJ
left a comment
Member
There was a problem hiding this comment.
Looks good to me, working nicely on my local setup.
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Description
Refactors are made to the azure entity descendant relationship data retrieval query logic for improving efficiency and query duration.
Motivation and Context
Resolves BED-4867
Our current methods for obtaining entity descendant relationship data often result in timeout errors in modestly sized tenants. This issue is exacerbated when attempting to view this information through the entity panel in the UI as a series of requests are executed through a promise.all call that hinges on all of the requests succeeding and otherwise fails to show any information even if only one of the requests fails.
The logic has been updated to pivot based on source and target kinds in order to route queries more effectively. Our modeled AZContains relationships include those that are only ever 1 hop away (like AZTenant to AZDevice) and those that may be several hops away (AZTenant to AZLogicApp will at minimum cross an AZSubscription and AZResourceGroup).
We were previously traversing any AZContains relationships then returning paths that had the target kind in the path. This means that we would follow AZContains to deep terminals for any target kind when most if not all traversals could be eliminated up front.
The logic has been updated to first pull the set of terminals by kind and tenant id then traversing backwards to the source for relationships that are nested. Direct relationships simply do an edge filter rather than a traversal.
Finally, the UI is updated to use promise.allSettled over promise.all so that successful nested sections are still displayed and functional even if a sibling section fails.
How Has This Been Tested?
API integration tests have been added. The azure example data was uploaded and various descendant entity panels were inspected (the AZTenant descendants section was of particular focus).
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit
Release Notes
New Features
Performance Improvements
Bug Fixes
Tests