Telemetry: Add project age#33910
Conversation
shilman
added
maintenance
|
View your CI Pipeline Execution ↗ for commit 35cc133
☁️ Nx Cloud last updated this comment at |
shilman
force-pushed
the
shilman/add-project-since
branch
from
57e395a to
bc9b9df
Compare
valentinpalkovic
requested review from
jonniebigodes and
kylegach
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a memoized getProjectSince() that runs a git command via executeCommandSync to obtain the repo's first commit date, exposes its timestamp as projectSince in telemetry context, replaces execSync usage with executeCommandSync, and extends tests and docs for the new behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI / Telemetry Sender
participant Anonymous as anonymous-id module
participant Exec as executeCommandSync
participant Git as git (local repo)
CLI->>Anonymous: build telemetry context
Anonymous->>Exec: run "git log --reverse --format=%cI"
Exec->>Git: spawn git process
Git-->>Exec: stdout (first-commit date) or error
Exec-->>Anonymous: stdout or failure
Anonymous->>Anonymous: parse stdout -> Date | undefined (memoize)
Anonymous-->>CLI: context including projectSince (ms) if available
CLI->>CLI: send telemetry payload
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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: 2
🧹 Nitpick comments (1)
code/core/src/telemetry/anonymous-id.ts (1)
57-79: Limit git log cost and memoize project age.
git log --reverseemits the full history every call, andgetProjectSince()can be invoked per telemetry event. Limit output to a single commit and cache the result to avoid repeated process spawns/timeouts.♻️ Proposed refactor
+let projectSinceCache: Date | undefined; +let projectSinceResolved = false; + export const getProjectSince = () => { + if (projectSinceResolved) { + return projectSinceCache; + } + projectSinceResolved = true; + try { const dateBuffer = executeCommandSync({ command: 'git', - args: ['log', '--reverse', '--format=%cd', '--date=iso'], + args: ['log', '--reverse', '--max-count=1', '--format=%cd', '--date=iso'], timeout: 1000, }); const firstLine = String(dateBuffer).trim().split('\n')[0]; const date = new Date(firstLine); if (Number.isNaN(date.getTime())) { - return undefined; + projectSinceCache = undefined; + return undefined; } - return date; + projectSinceCache = date; + return date; } catch (_) { // } - return undefined; + projectSinceCache = undefined; + return undefined; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/telemetry/anonymous-id.ts` around lines 57 - 79, getProjectSince currently shells out to git with --reverse and returns the first line each time; change it to request only the first commit (use git log --reverse --max-count=1 or git log -1 --format=%cd --date=iso) to avoid streaming full history, and memoize the computed Date (store it in a module-level variable, e.g., cachedProjectSince) so subsequent calls return the cached value instead of calling executeCommandSync again; keep the same error handling and return undefined on failure or invalid date.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code/core/src/telemetry/anonymous-id.test.ts`:
- Around line 1-14: The tests currently set per-test inline mocks and rely on
real git output; move the executeCommandSync mock setup into a beforeEach block
and use vi.mocked(executeCommandSync) to set return values there so all tests
get controlled, deterministic git output (mock the git command responses used by
getProjectSince/normalizeGitUrl/unhashedProjectId); remove per-test mock
implementations (lines 125–137) and instead in beforeEach call
vi.mocked(executeCommandSync).mockImplementation(...) or .mockReturnValue(...)
to stabilize the git-dependent test — keep the async factory vi.mock(...) as-is
and do not add the { spy: true } option.
In `@docs/configure/telemetry.mdx`:
- Around line 65-71: The example payload's "projectSince" is shown as a string
but the implementation sends a numeric timestamp (Date.getTime()); update the
example JSON so "projectSince" is a number (no quotes) and uses a representative
numeric value (e.g. 1717334400000) to match the actual behavior and avoid
confusion—modify the JSON example containing "context" and the "projectSince"
field accordingly.
---
Nitpick comments:
In `@code/core/src/telemetry/anonymous-id.ts`:
- Around line 57-79: getProjectSince currently shells out to git with --reverse
and returns the first line each time; change it to request only the first commit
(use git log --reverse --max-count=1 or git log -1 --format=%cd --date=iso) to
avoid streaming full history, and memoize the computed Date (store it in a
module-level variable, e.g., cachedProjectSince) so subsequent calls return the
cached value instead of calling executeCommandSync again; keep the same error
handling and return undefined on failure or invalid date.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
code/core/src/telemetry/anonymous-id.test.tscode/core/src/telemetry/anonymous-id.tscode/core/src/telemetry/telemetry.tsdocs/configure/telemetry.mdx
| import { describe, expect, it, vi } from 'vitest'; | ||
|
|
||
| import { normalizeGitUrl, unhashedProjectId } from './anonymous-id'; | ||
| import { executeCommandSync } from 'storybook/internal/common'; | ||
|
|
||
| import { getProjectSince, normalizeGitUrl, unhashedProjectId } from './anonymous-id'; | ||
|
|
||
| vi.mock(import('storybook/internal/common'), async (actualModule) => { | ||
| const actual = await actualModule(); | ||
|
|
||
| return { | ||
| ...actual, | ||
| executeCommandSync: vi.fn(actual.executeCommandSync), | ||
| }; | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Vitest vi.mock async factory with spy:true option syntax
💡 Result:
spy: true is an alternative to providing a factory—you can’t pass { spy: true } and an async factory in the same vi.mock() call (the 2nd argument is either MockOptions or MockFactory). [1]
Auto-spy the whole module (no factory)
import * as mod from './mod'
vi.mock('./mod', { spy: true })This keeps original implementations but wraps exports so you can assert calls. [2][1]
If you need an async “factory-like” partial mock, wrap originals as spies yourself
vi.mock('./mod', async (importOriginal) => {
const actual = await importOriginal<typeof import('./mod')>()
return {
...actual,
someFn: vi.fn(actual.someFn), // spy that still calls through
// or override others as needed
}
})Vitest allows async factories and gives you importOriginal to access the real module. [1]
Sources:
[1] Vitest vi.mock API reference (types/overloads; async factory support)
[2] Vitest “Mocking Modules” guide ({ spy: true } behavior and usage)
🏁 Script executed:
yarn lint:js:cmd code/core/src/telemetry/anonymous-id.test.ts 2>&1 || trueRepository: storybookjs/storybook
Length of output: 283
🏁 Script executed:
cat -n code/core/src/telemetry/anonymous-id.test.ts | head -150Repository: storybookjs/storybook
Length of output: 5332
Move mock implementations to beforeEach blocks and stabilize git-dependent test.
The current mock setup has inline mock implementations inside individual test cases (lines 125–137), which violates Vitest patterns. Additionally, the first test relies on real git history and will fail on shallow clones. Move all mock behavior to beforeEach, use vi.mocked() to access the mock, and mock the git output for all cases. The async factory pattern is valid as-is; do not add { spy: true } as that option is an alternative to the factory, not compatible with it.
Refactor mock setup
import { describe, expect, it, vi } from 'vitest';
+import { beforeEach } from 'vitest';
import { executeCommandSync } from 'storybook/internal/common';
import { getProjectSince, normalizeGitUrl, unhashedProjectId } from './anonymous-id';
vi.mock('storybook/internal/common', async (importOriginal) => {
- const actual = await importOriginal();
+ const actual = await importOriginal<typeof import('storybook/internal/common')>();
return {
...actual,
executeCommandSync: vi.fn(actual.executeCommandSync),
};
});
+
+const mockedExecuteCommandSync = vi.mocked(executeCommandSync);
+
+beforeEach(() => {
+ mockedExecuteCommandSync.mockReset();
+ mockedExecuteCommandSync.mockReturnValue('2015-12-11T10:54:01.000Z');
+});
describe('getProjectSince', () => {
it('returns the Storybook creation date from git log output', () => {
expect(getProjectSince()).toEqual(new Date('2015-12-11T10:54:01.000Z'));
});
it('returns undefined if git log output is empty', () => {
- vi.mocked(executeCommandSync).mockReturnValue('');
+ mockedExecuteCommandSync.mockReturnValue('');
expect(getProjectSince()).toBeUndefined();
});
it('returns undefined if git log fails', () => {
- vi.mocked(executeCommandSync).mockImplementation(() => {
+ mockedExecuteCommandSync.mockImplementation(() => {
throw new Error('git not available');
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@code/core/src/telemetry/anonymous-id.test.ts` around lines 1 - 14, The tests
currently set per-test inline mocks and rely on real git output; move the
executeCommandSync mock setup into a beforeEach block and use
vi.mocked(executeCommandSync) to set return values there so all tests get
controlled, deterministic git output (mock the git command responses used by
getProjectSince/normalizeGitUrl/unhashedProjectId); remove per-test mock
implementations (lines 125–137) and instead in beforeEach call
vi.mocked(executeCommandSync).mockImplementation(...) or .mockReturnValue(...)
to stabilize the git-dependent test — keep the async factory vi.mock(...) as-is
and do not add the { spy: true } option.
There was a problem hiding this comment.
Pull request overview
This PR adds telemetry for tracking project age by collecting the project creation date from git history. The implementation retrieves the timestamp of the first git commit to help understand whether Storybook is being installed into new or existing projects.
Changes:
- Added
getProjectSince()function to retrieve the project creation date from git log - Integrated
projectSincefield into telemetry context payload - Updated telemetry documentation to include the new field in example output
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
code/core/src/telemetry/anonymous-id.ts |
Added getProjectSince() function that executes git log --reverse --format=%cd --date=iso to get the first commit date |
code/core/src/telemetry/telemetry.ts |
Integrated projectSince into the telemetry context by calling getProjectSince()?.getTime() |
code/core/src/telemetry/anonymous-id.test.ts |
Added comprehensive unit tests covering success, empty output, and error scenarios |
docs/configure/telemetry.mdx |
Updated example telemetry output to show the new projectSince field in the context object |
| "nodeVersion": "24.11.0", | ||
| "storybookVersion": "10.3.0-alpha.9", | ||
| "cliVersion": "10.3.0-alpha.9", | ||
| "projectSince": "1717334400000" |
There was a problem hiding this comment.
The projectSince value should be a number, not a string. Since the code uses getProjectSince()?.getTime() which returns a number (or undefined), and this is serialized with JSON.stringify, the value will appear as a number in the JSON output. The documentation should show it as "projectSince": 1717334400000 instead of "projectSince": "1717334400000".
| "projectSince": "1717334400000" | |
| "projectSince": 1717334400000 |
| describe('getProjectSince', () => { | ||
| it('returns the Storybook creation date from git log output', () => { | ||
| expect(getProjectSince()).toEqual(new Date('2015-12-11T10:54:01.000Z')); | ||
| }); | ||
|
|
||
| it('returns undefined if git log output is empty', () => { | ||
| vi.mocked(executeCommandSync).mockReturnValue(''); | ||
|
|
||
| expect(getProjectSince()).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('returns undefined if git log fails', () => { | ||
| vi.mocked(executeCommandSync).mockImplementation(() => { | ||
| throw new Error('git not available'); | ||
| }); | ||
|
|
||
| expect(getProjectSince()).toBeUndefined(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Consider adding a test case to verify that when git log returns multiple lines, only the first line is parsed. This would ensure the implementation correctly handles the typical multi-commit scenario. For example:
vi.mocked(executeCommandSync).mockReturnValue('2015-12-11 10:54:01 +0000\n2016-01-01 12:00:00 +0000');
expect(getProjectSince()).toEqual(new Date('2015-12-11T10:54:01.000Z'));
| describe('getProjectSince', () => { | ||
| it('returns the Storybook creation date from git log output', () => { | ||
| expect(getProjectSince()).toEqual(new Date('2015-12-11T10:54:01.000Z')); | ||
| }); | ||
|
|
||
| it('returns undefined if git log output is empty', () => { | ||
| vi.mocked(executeCommandSync).mockReturnValue(''); | ||
|
|
||
| expect(getProjectSince()).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('returns undefined if git log fails', () => { | ||
| vi.mocked(executeCommandSync).mockImplementation(() => { | ||
| throw new Error('git not available'); | ||
| }); | ||
|
|
||
| expect(getProjectSince()).toBeUndefined(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Consider adding a test case for when git log returns an invalid date string to verify that the NaN check at line 69 properly handles this edge case. For example:
vi.mocked(executeCommandSync).mockReturnValue('not a valid date');
expect(getProjectSince()).toBeUndefined();
There was a problem hiding this comment.
♻️ Duplicate comments (2)
code/core/src/telemetry/anonymous-id.test.ts (2)
137-139: Mock implementations still set inline within test cases.All five sites set
vi.mocked(executeCommandSync)behaviors inside individualitcallbacks rather than inbeforeEach. This violates the project's Vitest mocking guidelines.Based on learnings: "Implement mock behaviors in
beforeEachblocks in Vitest tests."Also applies to: 145-145, 153-155, 172-172, 181-183
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/telemetry/anonymous-id.test.ts` around lines 137 - 139, Several tests in anonymous-id.test.ts set vi.mocked(executeCommandSync) inside individual it callbacks; move these mock implementations into a shared beforeEach to follow Vitest guidelines. Create a beforeEach that calls vi.mocked(executeCommandSync) and sets the appropriate return values (use mockReturnValue or mockReturnValueOnce for sequences), include vi.resetAllMocks() or vi.clearAllMocks() at the start of beforeEach to isolate tests, and remove the inline vi.mocked(executeCommandSync)... calls from the it blocks (adjust expected values in each test to rely on the beforeEach-provided mock behavior). Ensure you reference the existing executeCommandSync mocked symbol and use the same test case names so behavior remains consistent.
136-142:⚠️ Potential issue | 🟡 MinorTest 1 in each
describeblock will fail when tests run out of order due to memoized cache.Both
getProjectSince(lines 61–63, 79) andgetAnonymousProjectId(lines 38–40, 51) use module-level caching. The static imports at the top of the test file (lines 5–10) are bound at module load time and are not reset byvi.resetModules()inbeforeEach. This means:
- Test 1 in each describe calls the statically-imported function whose cache persists across test runs
- Tests 2 & 3 correctly use dynamic imports to fetch fresh module instances with empty caches
- In watch mode, on retry, or when test order changes (e.g.,
.only), test 1 will return the stale cached value, causing failuresApply the same
vi.resetModules()+ dynamic-import pattern to test 1 of each describe:Fix for getProjectSince test 1 (lines 136–142)
it('returns the Storybook creation date from git log output', () => { vi.mocked(executeCommandSync).mockReturnValue( '2025-12-11 16:24:01 +0530\n' + '2014-12-11 19:09:10 +0530' ); - expect(getProjectSince()).toEqual(new Date('2025-12-11T10:54:01.000Z')); + const { getProjectSince: getProjSince } = await import('./anonymous-id'); + expect(getProjSince()).toEqual(new Date('2025-12-11T10:54:01.000Z')); });Fix for getAnonymousProjectId test 1 (lines 171–176)
it('returns hashed project id for Storybook repo when git command succeeds', async () => { vi.mocked(executeCommandSync).mockReturnValue('git@github.com:storybookjs/storybook.git'); - const result = getAnonymousProjectId(); + const { getAnonymousProjectId: getAnonId } = await import('./anonymous-id'); + const result = getAnonId(); expect(result).toMatch('061e4ee22a1f7c079849d97234b3be94d016fb1f24ba11878c41f8b48c0213bf'); });Also move mock implementations from test bodies (lines 137–139, 145, 153–155, 172, 181–183) into
beforeEachblocks and restoreprocess.cwdspy (line 168) withvi.restoreAllMocks()inafterEach.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/telemetry/anonymous-id.test.ts` around lines 136 - 142, The failing-first-test is caused by module-level memoized caches in getProjectSince and getAnonymousProjectId persisting across tests; update the first test in each describe to call vi.resetModules() and dynamically import the module (e.g., const { getProjectSince } = await import('...') / const { getAnonymousProjectId } = await import('...')) so you get a fresh module instance, move executeCommandSync and other mock implementations currently inside those test bodies into the describe-level beforeEach to centralize mocking, and ensure cleanup by calling vi.restoreAllMocks() (and vi.resetModules() as needed) in afterEach so process.cwd spy and other mocks are restored between tests.
🧹 Nitpick comments (1)
code/core/src/telemetry/anonymous-id.test.ts (1)
171-171: Unnecessaryasynckeyword — but will be needed after the dynamic-import fix.The test body contains no
await, makingasyncredundant. However, once the static-import fix described above is applied (addingawait import('./anonymous-id')),asyncwill be required. No action needed if that fix is adopted.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/telemetry/anonymous-id.test.ts` at line 171, The test "returns hashed project id for Storybook repo when git command succeeds" currently has an unnecessary async on the it callback because there is no await; either remove the async from that it(...) declaration to avoid misleading async usage, or if you implement the earlier suggested dynamic import (await import('./anonymous-id')), keep the async and add the awaited dynamic import inside the test body so the async is required; locate the test by its description string to apply the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@code/core/src/telemetry/anonymous-id.test.ts`:
- Around line 137-139: Several tests in anonymous-id.test.ts set
vi.mocked(executeCommandSync) inside individual it callbacks; move these mock
implementations into a shared beforeEach to follow Vitest guidelines. Create a
beforeEach that calls vi.mocked(executeCommandSync) and sets the appropriate
return values (use mockReturnValue or mockReturnValueOnce for sequences),
include vi.resetAllMocks() or vi.clearAllMocks() at the start of beforeEach to
isolate tests, and remove the inline vi.mocked(executeCommandSync)... calls from
the it blocks (adjust expected values in each test to rely on the
beforeEach-provided mock behavior). Ensure you reference the existing
executeCommandSync mocked symbol and use the same test case names so behavior
remains consistent.
- Around line 136-142: The failing-first-test is caused by module-level memoized
caches in getProjectSince and getAnonymousProjectId persisting across tests;
update the first test in each describe to call vi.resetModules() and dynamically
import the module (e.g., const { getProjectSince } = await import('...') / const
{ getAnonymousProjectId } = await import('...')) so you get a fresh module
instance, move executeCommandSync and other mock implementations currently
inside those test bodies into the describe-level beforeEach to centralize
mocking, and ensure cleanup by calling vi.restoreAllMocks() (and
vi.resetModules() as needed) in afterEach so process.cwd spy and other mocks are
restored between tests.
---
Nitpick comments:
In `@code/core/src/telemetry/anonymous-id.test.ts`:
- Line 171: The test "returns hashed project id for Storybook repo when git
command succeeds" currently has an unnecessary async on the it callback because
there is no await; either remove the async from that it(...) declaration to
avoid misleading async usage, or if you implement the earlier suggested dynamic
import (await import('./anonymous-id')), keep the async and add the awaited
dynamic import inside the test body so the async is required; locate the test by
its description string to apply the change.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/core/src/telemetry/anonymous-id.test.tscode/core/src/telemetry/anonymous-id.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- code/core/src/telemetry/anonymous-id.ts
Telemetry: Add project age (cherry picked from commit 036d55a)
github-actions
Bot
added
the
patch:done
Closes N/A
What I did
Collect the project age so we can understand whether Storybook is being installed into new projects
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
create-storybookin a project withSTORYBOOK_TELEMETRY_DEBUG=1and verify the outputDocumentation
Updated
telemetry.mdxto include new data being collected🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=<PR_NUMBER>Summary by CodeRabbit
New Features
Tests
Documentation