feat(browser): add browser score ingestion package - #842
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
@claude review |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce4fa9b6c6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1c2b0a70be
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 30b093b13a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| headers: { | ||
| ...this.additionalHeaders, | ||
| "Content-Type": "application/json", | ||
| Authorization: `Bearer ${this.publicKey}`, | ||
| "X-Langfuse-Public-Key": this.publicKey, | ||
| "X-Langfuse-Sdk-Name": LANGFUSE_SDK_NAME, | ||
| "X-Langfuse-Sdk-Version": LANGFUSE_SDK_VERSION, | ||
| "X-Langfuse-Sdk-Integration": SDK_INTEGRATION, | ||
| }, |
There was a problem hiding this comment.
🔴 The unit test at tests/unit/browser.test.ts:62-69 — the same command the PR description lists as a verification step (pnpm --filter @langfuse/browser test) — fails on this branch. The test expects X-Langfuse-Sdk-Variant: 'langfuse-browser' (never emitted) and X-Langfuse-Sdk-Integration: 'DEFAULT', but packages/browser/src/index.ts:128-136 omits the variant header entirely and sets Integration to 'browser' from SDK_INTEGRATION at line 25. The reserved-header set at packages/browser/src/utils.ts:8 already lists 'x-langfuse-sdk-variant' — strongly suggesting the variant header was intended to be wired up but was missed. Either add the variant header and switch the Integration header value to 'DEFAULT' (keeping the payload metadata.sdk_integration as 'browser', which the test still expects), or update the test to match what the SDK actually sends.
Extended reasoning...
The bug
The PR's own stated verification step (pnpm --filter @langfuse/browser test, which resolves to vitest run --project=unit tests/unit/browser.test.ts) fails on this branch. One of the nine tests in tests/unit/browser.test.ts — sends a score as a single ingestion batch — asserts a header contract the SDK never emits.
Where the contradiction lives
Test expectations (tests/unit/browser.test.ts:62-69):
expect(init?.headers).toMatchObject({
"Content-Type": "application/json",
Authorization: "Bearer pk-lf-test",
"X-Langfuse-Public-Key": "pk-lf-test",
"X-Langfuse-Sdk-Name": "javascript",
"X-Langfuse-Sdk-Variant": "langfuse-browser", // never emitted
"X-Langfuse-Sdk-Integration": "DEFAULT", // SDK sends 'browser'
});SDK implementation (packages/browser/src/index.ts:128-136):
headers: {
...this.additionalHeaders,
"Content-Type": "application/json",
Authorization: `Bearer ${this.publicKey}`,
"X-Langfuse-Public-Key": this.publicKey,
"X-Langfuse-Sdk-Name": LANGFUSE_SDK_NAME,
"X-Langfuse-Sdk-Version": LANGFUSE_SDK_VERSION,
"X-Langfuse-Sdk-Integration": SDK_INTEGRATION, // line 25: const SDK_INTEGRATION = 'browser';
},No X-Langfuse-Sdk-Variant is ever set, and the integration value is the package identifier 'browser' rather than the canonical 'DEFAULT' the test expects.
Why existing code doesn't catch it
The reserved-header set at packages/browser/src/utils.ts:3-10 already lists 'x-langfuse-sdk-variant' alongside the other SDK headers — meaning the author specifically reserved that name to prevent caller overrides, yet never wired the SDK itself to emit it. This is the kind of half-wired feature that the failing test was clearly written to enforce. The payload metadata at line 120 (sdk_integration: SDK_INTEGRATION) is correct (test at line 76 expects sdk_integration: 'browser' in the payload body), so the intended split is: payload metadata carries the package identifier ('browser'), the canonical header carries 'DEFAULT', and a separate X-Langfuse-Sdk-Variant: 'langfuse-browser' header identifies the variant. The implementation never made that split.
Impact
The PR's Verification section explicitly lists pnpm --filter @langfuse/browser test as a passing step. That command does not pass on the current HEAD — landing the PR as-is would publish a package whose own verification step is broken, and CI on a workflow gated by that test would fail.
Step-by-step proof
- Check out the PR branch and run
./node_modules/.bin/vitest run --project=unit tests/unit/browser.test.ts. - Observe 1 of 9 tests failing:
LangfuseBrowser > sends a score as a single ingestion batch. - The diff from
toMatchObjectshows:- "X-Langfuse-Sdk-Integration": "DEFAULT", + "X-Langfuse-Sdk-Integration": "browser", - "X-Langfuse-Sdk-Variant": "langfuse-browser", - Source inspection confirms:
index.ts:25definesSDK_INTEGRATION = 'browser';index.ts:135uses it as the header value; no line setsX-Langfuse-Sdk-Variant.
How to fix
Either path is acceptable, but the variant-aware split is the more idiomatic one across SDK conventions:
- Wire the SDK to match the test (recommended): add
'X-Langfuse-Sdk-Variant': 'langfuse-browser'to the headers block atindex.ts:128-136, and change theX-Langfuse-Sdk-Integrationvalue to'DEFAULT'(keepSDK_INTEGRATION = 'browser'as the payload metadata value, which the test at line 76 still requires). - Or update the test to drop the variant expectation and assert
X-Langfuse-Sdk-Integration: 'browser'.
The reserved-header list and the fact the test was clearly authored against the variant contract suggest the first option is the intended design — but whichever direction the maintainer prefers, the PR should not merge with its own verification step broken.
2 participants
Summary
@langfuse/browserpackage for public-key browser score ingestion/api/public/ingestionusing Bearer public-key authImpacted packages
@langfuse/browserVerification
pnpm --filter @langfuse/browser testpnpm --filter @langfuse/browser buildpnpm buildpnpm typecheckpnpm lintpnpm format:checkNote:
pnpm buildstill prints existing generated-client duplicate-case warnings from@langfuse/core, but exits successfully.Greptile Summary
This PR introduces a new
@langfuse/browserpackage that provides a minimal browser-safe SDK for sending Langfuse scores from public-key environments, wiring it into the monorepo's build, test, docs, and TypeScript config.LangfuseBrowserclass wraps the/api/public/ingestionendpoint with Bearer public-key auth, immediate (non-batched) score dispatch, full request/response error handling, and header precedence for SDK telemetry fields.tsup,vitest(happy-dom),typedoc, and TypeScript project references are all added correctly; the package correctly exposes dual CJS/ESM builds.Confidence Score: 4/5
The package logic is sound and well-tested, but the version mismatch (5.4.1 vs. the rest of the monorepo at 5.5.0-beta.2) needs to be fixed before publishing.
The new package is self-contained, thoroughly tested, and the HTTP transport, error handling, and header precedence are all correct. The version mismatch in package.json would cause the new package to publish at a lower version than every other package in the suite, which will confuse release tooling and npm consumers. The two additional findings (fetch rebinding and || vs ??) are minor and do not affect the happy path.
packages/browser/package.json — version field needs to align with the rest of the monorepo before a release.
Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant App as Browser App participant SDK as LangfuseBrowser participant API as /api/public/ingestion App->>SDK: "score({ traceId, name, value, ... })" SDK->>SDK: "scoreId = body.id ?? generateUUID()" SDK->>SDK: "event = { id: envelopeUUID, type: score-create, body }" SDK->>API: POST /api/public/ingestion Note over SDK,API: Authorization: Bearer pk-lf-... Note over SDK,API: X-Langfuse-Public-Key, SDK telemetry headers alt HTTP error (non-2xx) API-->>SDK: 401 / 5xx SDK-->>App: throw LangfuseBrowserError (status, response) else Item-level error (207) API-->>SDK: "207 successes:[] errors:[{id, status, message}]" SDK-->>App: throw LangfuseBrowserError (errors[]) else Success (207) API-->>SDK: "207 successes:[{id: envelopeUUID, status:201}] errors:[]" SDK->>SDK: verify envelopeUUID in successes SDK-->>App: "{ id: scoreId }" end%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant App as Browser App participant SDK as LangfuseBrowser participant API as /api/public/ingestion App->>SDK: "score({ traceId, name, value, ... })" SDK->>SDK: "scoreId = body.id ?? generateUUID()" SDK->>SDK: "event = { id: envelopeUUID, type: score-create, body }" SDK->>API: POST /api/public/ingestion Note over SDK,API: Authorization: Bearer pk-lf-... Note over SDK,API: X-Langfuse-Public-Key, SDK telemetry headers alt HTTP error (non-2xx) API-->>SDK: 401 / 5xx SDK-->>App: throw LangfuseBrowserError (status, response) else Item-level error (207) API-->>SDK: "207 successes:[] errors:[{id, status, message}]" SDK-->>App: throw LangfuseBrowserError (errors[]) else Success (207) API-->>SDK: "207 successes:[{id: envelopeUUID, status:201}] errors:[]" SDK->>SDK: verify envelopeUUID in successes SDK-->>App: "{ id: scoreId }" endPrompt To Fix All With AI
Reviews (1): Last reviewed commit: "Merge branch 'main' into hassiebbot/add-..." | Re-trigger Greptile