refactor(sdk): shared @maple/sdk-core for the browser and Effect SDKs - #1167
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (47)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
918e9cd to
2e63c20
Compare
Makisuo
changed the title
Maple review🟡 Confidence 3/5 · needs attention Warning This review ended early; what follows is what it established. Extracts a private
What was checked
Observability coverage: 2 of 2 changes observable
Files not reviewed (1)The review ended before it read these diffs, so nothing above vouches for them.
|
…ct SDKs One framework-free implementation of the browser signal behaviour both published SDKs need, so they cannot drift. @maple/sdk-core (private, bundled into each SDK like browser-session): - root, runtime-agnostic: per-session sampling decision (ot=th/rv), error filter factory, Caused-by rendering that also walks Error-shaped copies (Effect's pretty errors), renders plain `errors` members by message and never throws on hostile objects, the HTTP status policy (status and network-failure messages), a header allowlist, the neutral log record, shared option groups and their defaults, and page-wide coordination under a versioned global: one reported-error set, an error-recorded bus, and a lease per page option owned by the bundled copy, so no option is collected twice and each SDK keeps its settings. - ./browser/*: web vitals, breadcrumbs, CSP/reporting, jank, document timing and the IndexedDB offline queue, emitting through injected sinks. Offline batches carry their endpoint and a credential fingerprint, so one SDK never resends another's batch under its key. - an architecture test forbids effect, @opentelemetry and anything that reaches rrweb, in either quote style, and keeps the root tier free of DOM imports and DOM globals. @maple-dev/browser now adapts these to OTel instead of owning them. The eager and first-party size ceilings rise by 0.5 kB for the page-wide coordination, with the reason in scripts/size.ts.
2e63c20 to
8b780d7
Compare
… into feat/sdk-parity
- sdk-core joins browser and browser-session in the no-try-catch exemption: it ships inside them with no runtime dependencies, so try/catch is the only error handling it has. - Attribute readers are typed (AttributeValue, ReadAttribute) instead of returning unknown, and the header filter takes and returns them. - Console level severities are a Map, so no string reaches the prototype. - page.test uses two small test seams instead of Reflect.get on globalThis. - openNavigationSpan is gone: jank parenting uses navigationSpanAt. - knip: web-vitals, like rrweb, is imported by a bundled private package and stays listed so the build keeps it external.
Maple reviewNothing to review Warning This review ended early; what follows is what it established. Review could not be completed: every tool call (pr_changed_files, pr_context) failed against GitHub, so no diff was read. The change extracts a shared What was checked
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
… into feat/sdk-parity # Conflicts: # packages/browser/src/deferred/perf.browser.test.ts # packages/sdk-core/src/browser/offline.ts
|
Note A newer push replaced |
CodeQL flagged the frame URL pattern as polynomial on page-controlled input (a stack line of `at a://` followed by many `@a://`). Frames are now parsed by position: take the location after the last `(`, `at ` or `@`, strip `:line[:col]` from the end, then check the scheme with a regex anchored at the start. Same results for V8, SpiderMonkey and JavaScriptCore frames; a test pins a 250 kB hostile line under 200 ms. The first-party size ceiling moves 19 -> 19.5 kB for the ~60 bytes the parser costs over the regex.
Maple review🟢 Confidence 4/5 · likely safe to merge Extracts
What was checked
|
2 participants
Stacked on #1168 (review fixes for the browser SDK); the diff is only this change.
What
First step of bringing
@maple-dev/browserand@maple-dev/effect-sdkto parity from one implementation instead of two.New private package
@maple/sdk-core, bundled into each SDK the same way@maple/browser-sessionis:ot=th/rvtracestateCaused by:rendering:Errors carrying copiedcause/errorserrorsonly on error-shaped values, and renders plain members (GraphQL-style{ message }) by their messagestack, throwing getters)METHOD url -> status|typemessage builder serves status errors and network failuresSignalLogRecord, plus the shared option groups (tracing,errors,webVitals,breadcrumbs,logs,reporting,transport,replay) and their defaultsglobalThiskey (a copy with another layout gets its own slot and never wipes this one):breadcrumbs,console,csp,browserReports,longFrames,slowInteractions,webVitals), owned by the bundled copy. No option is collected twice, and each SDK keeps the options the other doesn't collect../browser/*subpaths: web vitals, breadcrumbs, CSP/Reporting API, long frames and slow interactions, document timing, and the IndexedDB offline queue.emitLog,startSpan,stash) instead of calling OTel.effect,@opentelemetry/*and anything that reaches rrweb (including@maple/browser-session/replay), in either quote style orrequire;@maple-dev/browsernow adapts these to OTel (sampler, status exporter,exceptionOf, deferred-chunk wiring) instead of owning them.Why
The two SDKs had already drifted: the browser stack added bot exclusion, buffered replay, canvas and network bodies to one copy of the client session lifecycle and none to the other. Most stack features are export-time rules each SDK would otherwise reimplement.
Reviewer notes
Behaviour changes beyond the move:
window.onerrorspans now carrycode.column.number;responseStatusaccepts string status codes ("503");replay.onErrorSampleRatenow falls back to 0 instead of 1.Offline batches are scoped to their endpoint and key. After a key rotation or an endpoint change, batches stored under the old credentials are no longer resent under the new ones; they expire after 24h. That is the price of never sending one org's data with another org's key.
The Effect SDK does not import
sdk-coreyet. Its devDependency is left out until the PR that does.Size: the eager and first-party ceilings rise by 0.5 kB, with the reason in
scripts/size.ts. The page-wide coordination accounts for most of it.Tests:
@maple/sdk-core: 45 tests@maple-dev/browser: 124 tests@maple/browser-session: 234 tests@maple-dev/effect-sdkclient: 37 testsFollow-ups (separate PRs): the Effect SDK side:
session.id/user.id/event names on logs🤖 Generated with Claude Code
Summary by CodeRabbit