feat(cli-analytics): scaffold @posthog/cli-analytics package#3890
Closed
DanielVisca wants to merge 1 commit into
Closed
feat(cli-analytics): scaffold @posthog/cli-analytics package#3890DanielVisca wants to merge 1 commit into
DanielVisca wants to merge 1 commit into
Conversation
New SDK for command-line tools, sibling to @posthog/mcp. This base sets up the package (build, lint, test config), shared types, the $cli_* event and property schema, and the id/logger primitives. Generated-By: PostHog Code Task-Id: b56cf1bb-97f1-4daf-a65c-01a2cdb12eaa
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Author
This was referenced Jun 18, 2026
Contributor
|
Contributor
📝 No Changeset FoundThis PR doesn't include a changeset. A changeset is required to release a new version. How to add a changesetRun this command and follow the prompts: pnpm changesetRemember: Never use |
Contributor
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
packages/cli-analytics/src/__tests__/ids.test.ts:4-8
Per project conventions, parameterised tests are preferred. The multiple `expect` assertions in `'prefixes generated ids'` are a natural fit for `it.each` — each prefix becomes an independent test case with its own pass/fail signal.
```suggestion
it.each(['evt', 'ses', 'anon'] as const)('prefixes generated id with %s', (prefix) => {
expect(newPrefixedId(prefix)).toMatch(new RegExp(`^${prefix}_`))
})
```
### Issue 2 of 3
packages/cli-analytics/src/extensions/logger.ts:12-16
**Module-level logger is a process-wide singleton**
`activeLogger` is module-level state shared across every instance of `CliAnalytics` in the same process. Two callers calling `instrument()` with different `logger` options will overwrite each other, and Jest's `clearMocks` won't reset it between tests. If the logger is an instance concern rather than a process-level facility, passing it through the call-site context (e.g., captured in a closure when the analytics instance is created) would isolate instances cleanly.
### Issue 3 of 3
packages/cli-analytics/src/extensions/ids.ts:32-33
The multipliers used here are not valid FNV primes and produce poor avalanche effect. `0x000001b3` (435) and `0x00000193` (403) are only the low 9 bits of the 64-bit FNV prime and the 32-bit FNV prime respectively — the critical high bits are missing. With these tiny multipliers, a `Math.imul` only propagates carries through ~9 bits of the 32-bit output, so most input bits have no effect on the upper half of each word. The correct 32-bit FNV-1a prime is `0x01000193` (16777619).
```suggestion
h1 = Math.imul(h1 ^ c, 0x01000193)
h2 = Math.imul(h2 ^ c, 0x01000193)
```
Reviews (1): Last reviewed commit: "feat(cli-analytics): scaffold @posthog/c..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+4
to
+8
| it('prefixes generated ids', () => { | ||
| expect(newPrefixedId('evt')).toMatch(/^evt_/) | ||
| expect(newPrefixedId('ses')).toMatch(/^ses_/) | ||
| expect(newPrefixedId('anon')).toMatch(/^anon_/) | ||
| }) |
Contributor
There was a problem hiding this comment.
expect assertions in 'prefixes generated ids' are a natural fit for it.each — each prefix becomes an independent test case with its own pass/fail signal.
Suggested change
| it('prefixes generated ids', () => { | |
| expect(newPrefixedId('evt')).toMatch(/^evt_/) | |
| expect(newPrefixedId('ses')).toMatch(/^ses_/) | |
| expect(newPrefixedId('anon')).toMatch(/^anon_/) | |
| }) | |
| it.each(['evt', 'ses', 'anon'] as const)('prefixes generated id with %s', (prefix) => { | |
| expect(newPrefixedId(prefix)).toMatch(new RegExp(`^${prefix}_`)) | |
| }) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/cli-analytics/src/__tests__/ids.test.ts
Line: 4-8
Comment:
Per project conventions, parameterised tests are preferred. The multiple `expect` assertions in `'prefixes generated ids'` are a natural fit for `it.each` — each prefix becomes an independent test case with its own pass/fail signal.
```suggestion
it.each(['evt', 'ses', 'anon'] as const)('prefixes generated id with %s', (prefix) => {
expect(newPrefixedId(prefix)).toMatch(new RegExp(`^${prefix}_`))
})
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Comment on lines
+12
to
+16
| let activeLogger: LoggerFn | undefined | ||
|
|
||
| export function setLogger(logger: LoggerFn | undefined): void { | ||
| activeLogger = logger | ||
| } |
Contributor
There was a problem hiding this comment.
activeLogger is module-level state shared across every instance of CliAnalytics in the same process. Two callers calling instrument() with different logger options will overwrite each other, and Jest's clearMocks won't reset it between tests. If the logger is an instance concern rather than a process-level facility, passing it through the call-site context (e.g., captured in a closure when the analytics instance is created) would isolate instances cleanly.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/cli-analytics/src/extensions/logger.ts
Line: 12-16
Comment:
**Module-level logger is a process-wide singleton**
`activeLogger` is module-level state shared across every instance of `CliAnalytics` in the same process. Two callers calling `instrument()` with different `logger` options will overwrite each other, and Jest's `clearMocks` won't reset it between tests. If the logger is an instance concern rather than a process-level facility, passing it through the call-site context (e.g., captured in a closure when the analytics instance is created) would isolate instances cleanly.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+32
to
+33
| h1 = Math.imul(h1 ^ c, 0x000001b3) | ||
| h2 = Math.imul(h2 ^ c, 0x00000193) |
Contributor
There was a problem hiding this comment.
0x000001b3 (435) and 0x00000193 (403) are only the low 9 bits of the 64-bit FNV prime and the 32-bit FNV prime respectively — the critical high bits are missing. With these tiny multipliers, a Math.imul only propagates carries through ~9 bits of the 32-bit output, so most input bits have no effect on the upper half of each word. The correct 32-bit FNV-1a prime is 0x01000193 (16777619).
Suggested change
| h1 = Math.imul(h1 ^ c, 0x000001b3) | |
| h2 = Math.imul(h2 ^ c, 0x00000193) | |
| h1 = Math.imul(h1 ^ c, 0x01000193) | |
| h2 = Math.imul(h2 ^ c, 0x01000193) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/cli-analytics/src/extensions/ids.ts
Line: 32-33
Comment:
The multipliers used here are not valid FNV primes and produce poor avalanche effect. `0x000001b3` (435) and `0x00000193` (403) are only the low 9 bits of the 64-bit FNV prime and the 32-bit FNV prime respectively — the critical high bits are missing. With these tiny multipliers, a `Math.imul` only propagates carries through ~9 bits of the 32-bit output, so most input bits have no effect on the upper half of each word. The correct 32-bit FNV-1a prime is `0x01000193` (16777619).
```suggestion
h1 = Math.imul(h1 ^ c, 0x01000193)
h2 = Math.imul(h2 ^ c, 0x01000193)
```
How can I resolve this? If you propose a fix, please make it concise.
Contributor
Contributor
|
Size Change: 0 B Total Size: 17 MB ℹ️ View Unchanged
|
Contributor
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
Contributor
|
This PR was closed due to lack of activity. Feel free to reopen if it's still relevant. |
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Problem
Changes
Release info Sub-libraries affected
Libraries affected
Checklist
If releasing new changes
pnpm changesetto generate a changeset file🤖 Agent context
Autonomy: Human-driven (agent-assisted) — or — Fully autonomous
Created with PostHog Code