Conversation
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
📝 WalkthroughWalkthroughAdds timestamp-based filename generation to graph exports, replacing the static 'bh-graph.json' filename with dynamic names like 'bh-graph-YYYY-MM-DD_HH-MM-SS.json'. Includes a new helper function and comprehensive test coverage for the export utilities. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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 Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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.
| a.remove(); | ||
| }; | ||
|
|
||
| export const getDefaultGraphExportFileName = () => { |
There was a problem hiding this comment.
Thanks for the contribution!
We use luxon internally to handle datetime related logic since that tends to be cumbersome to do without a package. I would recommend reaching for that here so that the formatting is more ergonomic.
| export const getDefaultGraphExportFileName = () => { | |
| export const getDefaultGraphExportFileName = () => { | |
| const formattedDate = DateTime.now().toFormat(LuxonFormat.DATETIME_WITHOUT_TIMEZONE) | |
| return `bh-graph-${formattedDate}.json`; | |
| }; |
There are some other formats to choose from if you have a preference.
There was a problem hiding this comment.
Will get this updated when I get a chance!
2 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