feat: Add file ingest warnings to file ingest table BED-6995 #2255
Conversation
sirisjo
added
the
user interface
WalkthroughAdds a Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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
🤖 Fix all issues with AI agents
In
`@packages/javascript/bh-shared-ui/src/components/FileIngestTable/FileIngestDetailsPanel.tsx`:
- Around line 58-77: In FileIngestDetailsPanel (the JSX that renders error and
warning blocks), fix the typo in the className on the header divs: replace the
incorrect 'font-b`old' occurrences with 'font-bold' for both the "Error
Message(s):" and "Warning(s):" header divs so the bold styling is applied
correctly.
🧹 Nitpick comments (2)
packages/javascript/bh-shared-ui/src/components/FileIngestTable/FileIngestTable.test.tsx (2)
137-138: Consider moving the spy setup intobeforeEachor per-test for better isolation.Setting up the mock at module level applies it to all tests. If future tests need different return values, this global mock could interfere. Moving it to
beforeEachwith reset inafterEach, or configuring it per-test, would improve test isolation.♻️ Suggested refactor
-const useFileUploadQuerySpy = vi.spyOn(useFileUpload, 'useFileUploadQuery'); -useFileUploadQuerySpy.mockReturnValue({ data: MOCK_COMPLETED_TASKS_RESPONSE, isSuccess: true } as any); - describe('FileIngestTable', () => { + beforeEach(() => { + vi.spyOn(useFileUpload, 'useFileUploadQuery').mockReturnValue({ + data: MOCK_COMPLETED_TASKS_RESPONSE, + isSuccess: true, + } as any); + }); + it('shows a loading state', () => {
163-164: UsefindByRoleinstead ofgetByRolefor consistency with async testing.
getByRoleis synchronous and theawaiton line 163 has no effect. For async-rendered content, usefindByRolewhich returns a Promise and properly waits for the element.♻️ Proposed fix
- const jobID = await screen.getByRole('button', { name: 'View ingest 9 details' }); + const jobID = await screen.findByRole('button', { name: 'View ingest 9 details' }); await user.click(jobID);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/javascript/bh-shared-ui/src/components/DetailsAccordion/DetailsAccordion.tsxpackages/javascript/bh-shared-ui/src/components/FileIngestTable/FileIngestDetailsPanel.tsxpackages/javascript/bh-shared-ui/src/components/FileIngestTable/FileIngestTable.test.tsxpackages/javascript/js-client-library/src/types.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-06T21:23:30.689Z
Learnt from: dcairnsspecterops
Repo: SpecterOps/BloodHound PR: 2010
File: packages/javascript/bh-shared-ui/src/components/ExploreSearchCombobox/ExploreSearchCombobox.tsx:0-0
Timestamp: 2025-11-06T21:23:30.689Z
Learning: In ExploreSearchCombobox component (packages/javascript/bh-shared-ui/src/components/ExploreSearchCombobox/ExploreSearchCombobox.tsx), setting aria-autocomplete={null} on the TextField is intentional to override downshift's default aria-autocomplete attribute, which was flagged by Axe DevTools. This approach satisfies Axe accessibility requirements by removing the conflicting attribute.
Applied to files:
packages/javascript/bh-shared-ui/src/components/DetailsAccordion/DetailsAccordion.tsx
📚 Learning: 2026-01-08T22:00:44.868Z
Learnt from: TheNando
Repo: SpecterOps/BloodHound PR: 2230
File: packages/javascript/bh-shared-ui/src/utils/object.ts:17-19
Timestamp: 2026-01-08T22:00:44.868Z
Learning: In the bh-shared-ui package, ensure that public utilities, types, and components are exported from the package’s API surface so they can be consumed by parent projects. This applies broadly to all files under packages/javascript/bh-shared-ui/src, not just this specific file. Avoid exporting internal, implementation-only modules that aren’t meant for external consumption; document and surface stable APIs for downstream users.
Applied to files:
packages/javascript/bh-shared-ui/src/components/DetailsAccordion/DetailsAccordion.tsxpackages/javascript/bh-shared-ui/src/components/FileIngestTable/FileIngestDetailsPanel.tsxpackages/javascript/bh-shared-ui/src/components/FileIngestTable/FileIngestTable.test.tsx
🧬 Code graph analysis (2)
packages/javascript/bh-shared-ui/src/components/FileIngestTable/FileIngestDetailsPanel.tsx (4)
packages/javascript/js-client-library/src/types.ts (1)
FileIngestCompletedTask(583-589)packages/javascript/bh-shared-ui/src/types.ts (1)
IndicatorType(114-114)packages/javascript/bh-shared-ui/src/components/StatusIndicator.tsx (1)
StatusIndicator(40-52)packages/javascript/bh-shared-ui/src/components/DetailsAccordion/DetailsAccordion.tsx (1)
DetailsAccordion(75-135)
packages/javascript/bh-shared-ui/src/components/FileIngestTable/FileIngestTable.test.tsx (3)
packages/javascript/js-client-library/src/types.ts (1)
FileIngestJob(570-581)packages/javascript/js-client-library/src/responses.ts (1)
FileIngestCompletedTasksResponse(271-271)packages/javascript/bh-shared-ui/src/components/FileIngestTable/FileIngestTable.tsx (1)
FileIngestTable(68-116)
🔇 Additional comments (7)
packages/javascript/bh-shared-ui/src/components/DetailsAccordion/DetailsAccordion.tsx (1)
109-110: LGTM!Adding
data-testidtoAccordionItemimproves testability and aligns with the new test coverage for partially completed ingest jobs.packages/javascript/js-client-library/src/types.ts (1)
583-588: LGTM!The
warningsfield addition mirrors the existingerrorsfield structure and aligns with the backend API response update.packages/javascript/bh-shared-ui/src/components/FileIngestTable/FileIngestDetailsPanel.tsx (3)
25-47: LGTM!The status and label derivation logic correctly handles the three states: success (no errors/warnings), partial success (warnings only), and failure (has errors). The mapping to
IndicatorTypevalues is appropriate.
82-83: LGTM!The helper function correctly checks for both empty errors and warnings arrays, with proper null safety via optional chaining.
96-101: LGTM!The
itemDisabledprop usage correctly disables accordion expansion for files without errors or warnings, which is appropriate since there's nothing to display.packages/javascript/bh-shared-ui/src/components/FileIngestTable/FileIngestTable.test.tsx (2)
57-110: LGTM!The mock data for partial success jobs and completed tasks with warnings provides good test coverage for the new warning functionality.
166-180: LGTM!The test correctly exercises the warning display flow: selecting a job, expanding the accordion, and verifying the warning message is rendered.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
packages/javascript/bh-shared-ui/src/components/FileIngestTable/FileIngestDetailsPanel.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@packages/javascript/bh-shared-ui/src/components/FileIngestTable/FileIngestTable.test.tsx`:
- Around line 158-179: Replace non-awaiting queries with async find* variants in
the test: change the calls that fetch the job button and dropdown from
getByRole/getAllByTestId to findByRole/findAllByTestId so the test waits for
elements rendered after async actions; specifically update the variables in
FileIngestTable.test.tsx where jobID is obtained (currently using getByRole) and
where jobsDropdown is obtained (currently using getAllByTestId) to use
findByRole and findAllByTestId respectively, keeping the existing
await/user.click sequence unchanged.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/javascript/bh-shared-ui/src/components/DetailsAccordion/DetailsAccordion.tsxpackages/javascript/bh-shared-ui/src/components/FileIngestTable/FileIngestDetailsPanel.tsxpackages/javascript/bh-shared-ui/src/components/FileIngestTable/FileIngestTable.test.tsxpackages/javascript/js-client-library/src/types.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/javascript/js-client-library/src/types.ts
- packages/javascript/bh-shared-ui/src/components/DetailsAccordion/DetailsAccordion.tsx
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-01-08T22:00:44.868Z
Learnt from: TheNando
Repo: SpecterOps/BloodHound PR: 2230
File: packages/javascript/bh-shared-ui/src/utils/object.ts:17-19
Timestamp: 2026-01-08T22:00:44.868Z
Learning: In the bh-shared-ui package, ensure that public utilities, types, and components are exported from the package’s API surface so they can be consumed by parent projects. This applies broadly to all files under packages/javascript/bh-shared-ui/src, not just this specific file. Avoid exporting internal, implementation-only modules that aren’t meant for external consumption; document and surface stable APIs for downstream users.
Applied to files:
packages/javascript/bh-shared-ui/src/components/FileIngestTable/FileIngestTable.test.tsxpackages/javascript/bh-shared-ui/src/components/FileIngestTable/FileIngestDetailsPanel.tsx
📚 Learning: 2025-09-08T19:22:49.284Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabelDialog.tsx:34-35
Timestamp: 2025-09-08T19:22:49.284Z
Learning: In BloodHound's TagToZoneLabelDialog component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabelDialog.tsx), importing AssetGroupTag type from 'js-client-library' to type tag shapes is incorrect - this type should not be used for typing tags in this context.
Applied to files:
packages/javascript/bh-shared-ui/src/components/FileIngestTable/FileIngestDetailsPanel.tsx
📚 Learning: 2025-09-08T19:38:54.755Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/ConfirmUpdateQueryDialog.tsx:27-40
Timestamp: 2025-09-08T19:38:54.755Z
Learning: In BloodHound's ConfirmUpdateQueryDialog component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/ConfirmUpdateQueryDialog.tsx), the nested Dialog structure with DialogTitle and DialogActions inside DialogContent (rather than as direct siblings of Dialog) is intentional design and should not be changed to follow conventional MUI patterns.
Applied to files:
packages/javascript/bh-shared-ui/src/components/FileIngestTable/FileIngestDetailsPanel.tsx
🧬 Code graph analysis (1)
packages/javascript/bh-shared-ui/src/components/FileIngestTable/FileIngestTable.test.tsx (2)
packages/javascript/js-client-library/src/types.ts (1)
FileIngestJob(572-583)packages/javascript/js-client-library/src/responses.ts (1)
FileIngestCompletedTasksResponse(284-284)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: build-ui
- GitHub Check: run-analysis
- GitHub Check: run-tests
🔇 Additional comments (6)
packages/javascript/bh-shared-ui/src/components/FileIngestTable/FileIngestDetailsPanel.tsx (3)
18-22: LGTM — imports align with the new warning UI.
102-107: LGTM — itemDisabled correctly reflects errors + warnings.
27-89: No changes needed. TheFileIngestCompletedTasktype definition requires botherrorsandwarningsas non-optionalstring[]fields, and TypeScript's strict mode prevents these from being missing. The three component functions that accept non-nullableFileIngestCompletedTaskprops are guaranteed to receive valid arrays. The one function receiving a nullable value (isErrorAndWarningFree) already correctly uses optional chaining. Defensive defaults are unnecessary and would mask type errors.Likely an incorrect or invalid review comment.
packages/javascript/bh-shared-ui/src/components/FileIngestTable/FileIngestTable.test.tsx (3)
17-21: LGTM — test imports are appropriate for the new coverage.
57-110: LGTM — mock data covers partial success + warnings well.
137-138: LGTM — hook mock aligns with the new warning scenario.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| it('shows a table with partially completed jobs', async () => { | ||
| checkPermissionMock.mockImplementation(() => true); | ||
| await act(async () => render(<FileIngestTable />)); | ||
| const user = userEvent.setup(); | ||
|
|
||
| const jobID = await screen.getByRole('button', { name: 'View ingest 9 details' }); | ||
| await user.click(jobID); | ||
|
|
||
| const jobsDropdown = await screen.getAllByTestId('0'); | ||
| expect(jobsDropdown[0]).toBeInTheDocument(); | ||
|
|
||
| await user.click(jobsDropdown[0]); | ||
|
|
||
| const partiallyCompletedJob = await screen.findByText('generic-with-failed-edges.json'); | ||
| expect(partiallyCompletedJob).toBeInTheDocument(); | ||
|
|
||
| await user.click(partiallyCompletedJob); | ||
|
|
||
| const warningText = await screen.findByText( | ||
| 'skipping invalid relationship. unable to resolve endpoints. source: NON2@EXISTING.NODE, target: NON1@EXISTING.NODE' | ||
| ); | ||
| expect(warningText).toBeInTheDocument(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd packages/javascript/bh-shared-ui && find . -name "FileIngestTable.test.tsx" -type fRepository: SpecterOps/BloodHound
Length of output: 122
🏁 Script executed:
cat -n packages/javascript/bh-shared-ui/src/components/FileIngestTable/FileIngestTable.test.tsx | head -50Repository: SpecterOps/BloodHound
Length of output: 2141
🏁 Script executed:
cat -n packages/javascript/bh-shared-ui/src/components/FileIngestTable/FileIngestTable.test.tsx | sed -n '158,179p'Repository: SpecterOps/BloodHound
Length of output: 1182
🏁 Script executed:
# Check test-utils location and what it exports
find packages/javascript/bh-shared-ui -name "test-utils*" -type fRepository: SpecterOps/BloodHound
Length of output: 116
🏁 Script executed:
# Check imports at the top of FileIngestTable.test.tsx
cat -n packages/javascript/bh-shared-ui/src/components/FileIngestTable/FileIngestTable.test.tsx | sed -n '1,20p'Repository: SpecterOps/BloodHound
Length of output: 1035
🏁 Script executed:
# Check similar patterns in other test files
rg "screen\.(getByRole|getAllByTestId)" packages/javascript/bh-shared-ui/src/components --type tsx -n | head -20Repository: SpecterOps/BloodHound
Length of output: 92
🏁 Script executed:
# Check for findByRole/findAllByTestId usage in test files
rg "screen\.(findByRole|findAllByTestId)" packages/javascript/bh-shared-ui/src/components --type tsx -n | head -20Repository: SpecterOps/BloodHound
Length of output: 92
🏁 Script executed:
cat -n packages/javascript/bh-shared-ui/src/test-utils.jsxRepository: SpecterOps/BloodHound
Length of output: 3763
🏁 Script executed:
# Try with correct file extension
rg "screen\.(getByRole|getAllByTestId)" packages/javascript/bh-shared-ui/src/components -A 2 | head -30Repository: SpecterOps/BloodHound
Length of output: 3405
🏁 Script executed:
# Check for findByRole/findAllByTestId usage patterns
rg "screen\.find" packages/javascript/bh-shared-ui/src/components -A 1 | head -20Repository: SpecterOps/BloodHound
Length of output: 1938
🏁 Script executed:
# Check what screen methods are being used most frequently
rg "screen\.(get|find|query)" packages/javascript/bh-shared-ui/src/components --no-heading | sed 's/.*screen\./screen\./' | cut -d'(' -f1 | sort | uniq -c | sort -rn | head -15Repository: SpecterOps/BloodHound
Length of output: 489
Use async queries for elements that appear after async operations.
The button and dropdown elements are obtained after async render and user interactions; getByRole/getAllByTestId do not wait for elements to appear. Use findByRole/findAllByTestId instead to ensure elements are present before querying, preventing potential race conditions. This is consistent with the async pattern used elsewhere in the same test (lines 171, 176).
Suggested fix
- const jobID = await screen.getByRole('button', { name: 'View ingest 9 details' });
+ const jobID = await screen.findByRole('button', { name: 'View ingest 9 details' });
@@
- const jobsDropdown = await screen.getAllByTestId('0');
+ const jobsDropdown = await screen.findAllByTestId('0');🤖 Prompt for AI Agents
In
`@packages/javascript/bh-shared-ui/src/components/FileIngestTable/FileIngestTable.test.tsx`
around lines 158 - 179, Replace non-awaiting queries with async find* variants
in the test: change the calls that fetch the job button and dropdown from
getByRole/getAllByTestId to findByRole/findAllByTestId so the test waits for
elements rendered after async actions; specifically update the variables in
FileIngestTable.test.tsx where jobID is obtained (currently using getByRole) and
where jobsDropdown is obtained (currently using getAllByTestId) to use
findByRole and findAllByTestId respectively, keeping the existing
await/user.click sequence unchanged.
| import { AlertTitle } from '@mui/material'; | ||
| import Alert from '@mui/material/Alert'; |
There was a problem hiding this comment.
| import { AlertTitle } from '@mui/material'; | |
| import Alert from '@mui/material/Alert'; | |
| import { Alert, AlertTitle } from '@mui/material'; |
| } | ||
| return 'bad'; | ||
| })(); | ||
| const label: string = (() => { |
There was a problem hiding this comment.
| const label: string = (() => { | |
| const label = (() => { |
we can let ts infer here
| </div> | ||
| ))} | ||
| {errors.length > 0 && ( | ||
| <> |
There was a problem hiding this comment.
Let's clean up these surrounding fragments please 🙏
| </> | ||
| )} | ||
| {warnings.length > 0 && ( | ||
| <> |
There was a problem hiding this comment.
Let's clean up these surrounding fragments please 🙏
| {errors.length > 0 && ( | ||
| <> | ||
| <Alert severity='error'> | ||
| <AlertTitle>{errors.length === 1 ? 'Error Message:' : 'Error Messages:'}</AlertTitle> |
There was a problem hiding this comment.
Nice update for conditional pluralization!
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@packages/javascript/bh-shared-ui/src/components/FileIngestTable/FileIngestDetailsPanel.tsx`:
- Around line 69-77: Fix the inconsistent AlertTitle by appending a colon when
multiple warnings are shown: in the FileIngestDetailsPanel component where
AlertTitle currently uses {warnings.length === 1 ? 'Warning:' : 'Warnings'},
change the plural branch to include the colon so it reads 'Warnings:' (ensure
the conditional that renders AlertTitle for warnings uses {warnings.length === 1
? 'Warning:' : 'Warnings:'} to match the Error Messages: style).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/javascript/bh-shared-ui/src/components/FileIngestTable/FileIngestDetailsPanel.tsx
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-08T19:22:49.284Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabelDialog.tsx:34-35
Timestamp: 2025-09-08T19:22:49.284Z
Learning: In BloodHound's TagToZoneLabelDialog component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabelDialog.tsx), importing AssetGroupTag type from 'js-client-library' to type tag shapes is incorrect - this type should not be used for typing tags in this context.
Applied to files:
packages/javascript/bh-shared-ui/src/components/FileIngestTable/FileIngestDetailsPanel.tsx
📚 Learning: 2025-09-08T19:38:54.755Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/ConfirmUpdateQueryDialog.tsx:27-40
Timestamp: 2025-09-08T19:38:54.755Z
Learning: In BloodHound's ConfirmUpdateQueryDialog component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/ConfirmUpdateQueryDialog.tsx), the nested Dialog structure with DialogTitle and DialogActions inside DialogContent (rather than as direct siblings of Dialog) is intentional design and should not be changed to follow conventional MUI patterns.
Applied to files:
packages/javascript/bh-shared-ui/src/components/FileIngestTable/FileIngestDetailsPanel.tsx
📚 Learning: 2026-01-08T22:00:44.868Z
Learnt from: TheNando
Repo: SpecterOps/BloodHound PR: 2230
File: packages/javascript/bh-shared-ui/src/utils/object.ts:17-19
Timestamp: 2026-01-08T22:00:44.868Z
Learning: In the bh-shared-ui package, ensure that public utilities, types, and components are exported from the package’s API surface so they can be consumed by parent projects. This applies broadly to all files under packages/javascript/bh-shared-ui/src, not just this specific file. Avoid exporting internal, implementation-only modules that aren’t meant for external consumption; document and surface stable APIs for downstream users.
Applied to files:
packages/javascript/bh-shared-ui/src/components/FileIngestTable/FileIngestDetailsPanel.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-analysis
- GitHub Check: build-ui
- GitHub Check: run-tests
🔇 Additional comments (5)
packages/javascript/bh-shared-ui/src/components/FileIngestTable/FileIngestDetailsPanel.tsx (5)
17-23: LGTM!Imports are appropriate for the new warnings functionality and status typing.
26-46: LGTM!The status and label computation logic is clean and correctly handles all three states: success (no issues), partial success (warnings only), and failure (errors present).
51-53: LGTM!Content rendering logic correctly triggers on either errors or warnings being present.
83-84: LGTM!Helper correctly checks both errors and warnings arrays with proper null safety via optional chaining.
97-102: LGTM!The
itemDisabledpredicate correctly disables accordion expansion for items that have no issues to display.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Description
The backend API response for the
completed-tasksendpoint was updated to include any warnings that came back for the job. These warnings are now displayed alongside any errors in the file ingest table.Motivation and Context
Resolves BED-6995
This changes is required to help users better understand issues in their uploaded files.
How Has This Been Tested?
Unit test added, and tested manually in the UI.
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.