feat: Generate timestamped filenames for graph exports: BED-7609#2388
feat: Generate timestamped filenames for graph exports: BED-7609#2388klamping wants to merge 7 commits intoSpecterOps:BED-7609from
Conversation
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
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)
📝 WalkthroughWalkthroughReplaces the static export filename with a UTC timestamp-based filesystem-safe name via a new helper, updates exportToJson to use it, adds tests for filename generation and JSON export behavior, ensures test environment provides Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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.
🧹 Nitpick comments (4)
packages/javascript/bh-shared-ui/src/utils/exportGraphData.test.ts (3)
5-15: Mock/timer cleanup is not failure-safe — move toafterEachBoth tests place cleanup (
vi.useRealTimers(),createElementSpy.mockRestore(), global restorations) at the end of the test body. If anyexpectassertion throws, those cleanup calls are skipped and fake timers / mocked globals leak into subsequent tests, which can cause flaky failures that are hard to diagnose.Prefer an
afterEachhook (or pair withbeforeEach) for all teardown:♻️ Suggested structural refactor
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; -import { describe, expect, it, vi } from 'vitest'; import * as exportUtils from './exportGraphData'; describe('exportGraphData', () => { + beforeEach(() => { + vi.useFakeTimers(); + vi.setSystemTime(new Date('2026-02-14T20:27:20.000Z')); + }); + + afterEach(() => { + vi.useRealTimers(); + vi.restoreAllMocks(); + vi.unstubAllGlobals(); + }); + it('generates a human-friendly default filename for graph exports', () => { - vi.useFakeTimers(); - vi.setSystemTime(new Date('2026-02-14T20:27:20.000Z')); - const name = exportUtils.getDefaultGraphExportFileName(); expect(name).toBe('bh-graph-2026-02-14_20-27-20.json'); - - vi.useRealTimers(); }); it('uses the default filename generator when exporting JSON', () => { - vi.useFakeTimers(); - vi.setSystemTime(new Date('2026-02-14T20:27:20.000Z')); // ... rest of setup and assertions only }); });Also applies to: 17-62
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/javascript/bh-shared-ui/src/utils/exportGraphData.test.ts` around lines 5 - 15, Current tests call vi.useRealTimers() and restore spies at the end of each test body which can be skipped on failure; refactor by adding an afterEach hook that calls vi.useRealTimers() and restores any mocks/spies (e.g. createElementSpy.mockRestore()) so cleanup always runs, and keep vi.useFakeTimers()/vi.setSystemTime(...) in the test or pair with beforeEach if you prefer; apply the same afterEach teardown for other tests in the file (lines ~17-62) that currently perform manual cleanup.
49-50: Add a guard assertion before accessingcreatedAnchorIf the
createElementspy fails to capture the anchor (e.g., spy not set up correctly),createdAnchor?.downloadsilently evaluates toundefined, and the failure message will be a generic type mismatch rather than "anchor was never captured."♻️ Proposed improvement
+ expect(createdAnchor).toBeDefined(); expect(createdAnchor?.download).toBe('bh-graph-2026-02-14_20-27-20.json');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/javascript/bh-shared-ui/src/utils/exportGraphData.test.ts` around lines 49 - 50, Add a guard assertion to ensure the anchor was actually captured before accessing its properties: check that the test variable createdAnchor (the element captured from the createElement spy) is defined/non-null (or an instance of HTMLAnchorElement) with an explicit expect(createdAnchor).toBeDefined() / toBeTruthy() (or expect(createdAnchor).not.toBeNull()) immediately before the existing expect(createdAnchor?.download)... assertion so failures clearly indicate the anchor was never captured rather than producing a generic type mismatch.
21-60: Prefervi.stubGlobal/vi.spyOnover manualObject.definePropertyfor global mocking
Object.definePropertywithoutconfigurable: truemay create a non-reconfigurable property ifwindow.URL.createObjectURL(orglobalThis.MouseEvent) does not already exist in the test environment. The subsequent restore call (lines 53–55, 57–60) would then throw aTypeError: Cannot redefine property. Vitest'svi.stubGlobalstores the original value automatically and restores it viavi.unstubAllGlobals(). ForURL.createObjectURLspecifically,vi.stubGlobal("URL", { createObjectURL: () => "..." })is a documented pattern.♻️ Suggested idiomatic Vitest mocking
- const originalCreateObjectURL = (window.URL as any).createObjectURL; - const createObjectUrlSpy = vi.fn(() => 'blob:mock'); - Object.defineProperty(window.URL, 'createObjectURL', { - value: createObjectUrlSpy, - writable: true, - }); + const createObjectUrlSpy = vi.fn(() => 'blob:mock'); + vi.stubGlobal('URL', { ...window.URL, createObjectURL: createObjectUrlSpy }); - const originalMouseEvent = globalThis.MouseEvent; - class MockMouseEvent extends Event { + class MockMouseEvent extends Event { constructor(type: string, _init?: any) { super(type); } } - Object.defineProperty(globalThis, 'MouseEvent', { - value: MockMouseEvent, - writable: true, - }); + vi.stubGlobal('MouseEvent', MockMouseEvent); exportUtils.exportToJson({ a: 1 }); expect(createdAnchor?.download).toBe('bh-graph-2026-02-14_20-27-20.json'); expect(createObjectUrlSpy).toHaveBeenCalled(); - createElementSpy.mockRestore(); - Object.defineProperty(window.URL, 'createObjectURL', { - value: originalCreateObjectURL, - writable: true, - }); - Object.defineProperty(globalThis, 'MouseEvent', { - value: originalMouseEvent, - writable: true, - }); - vi.useRealTimers(); // cleanup handled by afterEach: vi.restoreAllMocks(), vi.unstubAllGlobals(), vi.useRealTimers()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/javascript/bh-shared-ui/src/utils/exportGraphData.test.ts` around lines 21 - 60, Replace manual Object.defineProperty mocks for window.URL.createObjectURL and globalThis.MouseEvent with Vitest helpers: use vi.stubGlobal('URL', { createObjectURL: createObjectUrlSpy }) or vi.stubGlobal('URL', { createObjectURL: () => 'blob:mock' }) to stub createObjectURL and use vi.stubGlobal('MouseEvent', MockMouseEvent) to stub MouseEvent; keep the document.createElement spy (createElementSpy) but remove the manual restoration of URL/MouseEvent and rely on vi.unstubAllGlobals() (or vi.restoreAllMocks()/vi.resetAllMocks() as appropriate) to restore originals after calling exportUtils.exportToJson({ a: 1 }) and the expectations. Ensure createObjectUrlSpy and MockMouseEvent names remain so tests still assert createObjectUrlSpy calls and createdAnchor behavior.packages/javascript/bh-shared-ui/src/utils/exportGraphData.ts (1)
32-38: UTC timestamp in filename is correct; optional refactor for local time if preferredThe public API surface is properly configured —
getDefaultGraphExportFileNameis correctly re-exported throughutils/index.tsand the main packageindex.ts, so downstream consumers can import it directly.Regarding the timestamp:
toISOString()always outputs UTC regardless of user timezone, so a 3:30 PM EST export will show_20-30-00in the filename. This is unambiguous and fine for uniqueness, but may surprise users expecting local time. If local-time stamps are preferred, consider usingnew Date().toLocaleString(...)with explicit format tokens or a date library.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/javascript/bh-shared-ui/src/utils/exportGraphData.ts` around lines 32 - 38, The current getDefaultGraphExportFileName uses new Date().toISOString() which yields a UTC timestamp; if you want local-time filenames instead, modify getDefaultGraphExportFileName to format the current date/time in the local timezone (e.g., produce YYYY-MM-DD_HH-mm-ss without milliseconds) rather than using toISOString(); implement this by using Date methods or Intl.DateTimeFormat/toLocaleString with explicit options (or a date lib) to build the local date/time parts, replace the 'T' and ':' characters as done now, and return the same `bh-graph-${safe}.json` pattern so callers of getDefaultGraphExportFileName keep the same API.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/javascript/bh-shared-ui/src/utils/exportGraphData.test.ts`:
- Around line 5-15: Current tests call vi.useRealTimers() and restore spies at
the end of each test body which can be skipped on failure; refactor by adding an
afterEach hook that calls vi.useRealTimers() and restores any mocks/spies (e.g.
createElementSpy.mockRestore()) so cleanup always runs, and keep
vi.useFakeTimers()/vi.setSystemTime(...) in the test or pair with beforeEach if
you prefer; apply the same afterEach teardown for other tests in the file (lines
~17-62) that currently perform manual cleanup.
- Around line 49-50: Add a guard assertion to ensure the anchor was actually
captured before accessing its properties: check that the test variable
createdAnchor (the element captured from the createElement spy) is
defined/non-null (or an instance of HTMLAnchorElement) with an explicit
expect(createdAnchor).toBeDefined() / toBeTruthy() (or
expect(createdAnchor).not.toBeNull()) immediately before the existing
expect(createdAnchor?.download)... assertion so failures clearly indicate the
anchor was never captured rather than producing a generic type mismatch.
- Around line 21-60: Replace manual Object.defineProperty mocks for
window.URL.createObjectURL and globalThis.MouseEvent with Vitest helpers: use
vi.stubGlobal('URL', { createObjectURL: createObjectUrlSpy }) or
vi.stubGlobal('URL', { createObjectURL: () => 'blob:mock' }) to stub
createObjectURL and use vi.stubGlobal('MouseEvent', MockMouseEvent) to stub
MouseEvent; keep the document.createElement spy (createElementSpy) but remove
the manual restoration of URL/MouseEvent and rely on vi.unstubAllGlobals() (or
vi.restoreAllMocks()/vi.resetAllMocks() as appropriate) to restore originals
after calling exportUtils.exportToJson({ a: 1 }) and the expectations. Ensure
createObjectUrlSpy and MockMouseEvent names remain so tests still assert
createObjectUrlSpy calls and createdAnchor behavior.
In `@packages/javascript/bh-shared-ui/src/utils/exportGraphData.ts`:
- Around line 32-38: The current getDefaultGraphExportFileName uses new
Date().toISOString() which yields a UTC timestamp; if you want local-time
filenames instead, modify getDefaultGraphExportFileName to format the current
date/time in the local timezone (e.g., produce YYYY-MM-DD_HH-mm-ss without
milliseconds) rather than using toISOString(); implement this by using Date
methods or Intl.DateTimeFormat/toLocaleString with explicit options (or a date
lib) to build the local date/time parts, replace the 'T' and ':' characters as
done now, and return the same `bh-graph-${safe}.json` pattern so callers of
getDefaultGraphExportFileName keep the same API.
klamping
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/javascript/bh-shared-ui/src/utils/exportGraphData.ts (1)
20-33: Consider revoking the object URL to prevent memory leaks.The
createObjectURLcall creates a blob URL that persists until the page is unloaded or explicitly revoked. While browsers will eventually clean these up, it's best practice to revoke them after use, especially if exports happen frequently.♻️ Suggested improvement
export const downloadFile = ({ data, fileName, fileType }: { data: any; fileName: string; fileType: string }) => { const blob = new Blob([data], { type: fileType }); // create an anchor tag and dispatch a click event on it to trigger download const a = document.createElement('a'); a.download = fileName; - a.href = window.URL.createObjectURL(blob); + const url = window.URL.createObjectURL(blob); + a.href = url; const clickEvent = new MouseEvent('click', { view: window, bubbles: true, cancelable: true, }); a.dispatchEvent(clickEvent); a.remove(); + window.URL.revokeObjectURL(url); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/javascript/bh-shared-ui/src/utils/exportGraphData.ts` around lines 20 - 33, The downloadFile function currently creates a blob URL via window.URL.createObjectURL but never revokes it; update downloadFile to store the created URL in a variable (e.g., const url = window.URL.createObjectURL(blob)), set a.href = url, append the anchor to the document (document.body.appendChild(a)) if needed, trigger the click (a.click() or dispatchEvent), then call window.URL.revokeObjectURL(url) after the click (either immediately or in a short setTimeout/after a click handler) and finally remove the anchor (a.remove()); this ensures blob URLs are cleaned up and prevents memory leaks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/javascript/bh-shared-ui/src/utils/exportGraphData.ts`:
- Around line 20-33: The downloadFile function currently creates a blob URL via
window.URL.createObjectURL but never revokes it; update downloadFile to store
the created URL in a variable (e.g., const url =
window.URL.createObjectURL(blob)), set a.href = url, append the anchor to the
document (document.body.appendChild(a)) if needed, trigger the click (a.click()
or dispatchEvent), then call window.URL.revokeObjectURL(url) after the click
(either immediately or in a short setTimeout/after a click handler) and finally
remove the anchor (a.remove()); this ensures blob URLs are cleaned up and
prevents memory leaks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 93e793f5-19f0-46d9-95a9-b3e34d3e47bb
📒 Files selected for processing (4)
packages/javascript/bh-shared-ui/src/setupTests.tsxpackages/javascript/bh-shared-ui/src/utils/datetime.tspackages/javascript/bh-shared-ui/src/utils/exportGraphData.test.tspackages/javascript/bh-shared-ui/src/utils/exportGraphData.ts
|
Fyi, did a quick sanity check to validate this works locally after the changes. Still looking good 
|
|
Hey, this PR looks good, but will need to fix the failing CI checks before we can merge. Looks like theres some static code analysis warnings as well as some failing tests. |
rvazarkar
changed the title
|
Internal ticket for tracking: https://specterops.atlassian.net/browse/BED-7609 |
|
Lint issues should be fixed now. Will keep an eye on the build |
|
@klamping Not exactly sure what's going wrong, but I suspect you may need to merge main again |
|
@wes-mil Yeah, that was a very vague error in the test run. Merged in the latest and will see how this next run does. |
|
@klamping Going to try and debug what's going on here, I apologize for the hiccups with our CLA check |
|
@wes-mil Thanks... but I bet it's because two accounts made git commits... So mine has the CLA signed, but not the 'klamping-tester' one. Let me squash these commits as 'klamping' and see if that fixes the issue. |
|
@klamping It's actually an issue on our end, because of some heightened security measures we had to disable certain workflows for external contributors. We'll be able to get around this by merging your PR into a PR of my own, and then merging into main. I do still need you to squash commit under your main profile because all commits into BloodHound are required to be tied to a verified commit! |
|
@klamping Sorry forgot that you cannot merge, if you could squash your commits down I will merge it into my branch and then author the merge into the product preserving commit history (so you're credited!) |
|
@wes-mil I've been having issues trying to get the squash to work with the fact that I've merged in changes from main already... along with working from a different fork. I took the easier route of just creating a new branch and pulled in my changes for it, and pushed it up with a single commit (effectively squashing all the commits). It's here: Would you like me to open a new PR with this single commit and just copy over the initial info from this one? |
|
@klamping a new PR works just fine! One of our engineers just fixed the CLA check, so if your branch is up to date with main you can point your PR at |
|
Closed in favor of duplicate #2489 |
5 participants
Description
Updates the exported filenames to use a generated, human friendly timestamp so that multiple file downloads are more cleanly organized/named.
Motivation and Context
Resolves #1879
How Has This Been Tested?
Via unit tests and manually on my local server instance
Screenshots (optional):
Types of changes
It simply updates the filename of the downloaded JSON, however, I think it qualifies as breaking, as if someone is basing some sort of script on the downloaded filename, it will break that script.
Checklist:
Summary by CodeRabbit
New Features
Tests
Tests (Test Setup)