fix(browser): Fix missing traces for user feedback#19660
fix(browser): Fix missing traces for user feedback#19660andreiborza wants to merge 4 commits intodevelopfrom
Conversation
size-limit report 📦
|
s1gr1d
force-pushed
the
ab/user-feedback-traces
branch
from
5e4c528 to
a2cb952
Compare
s1gr1d
force-pushed
the
ab/user-feedback-traces
branch
from
2fb3a76 to
a5c830b
Compare
andreiborza
left a comment
andreiborza
left a comment
There was a problem hiding this comment.
LGTM, I can't approve since it's my pr 😅
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| const feedbackEvent = envelopeRequestParser((await feedbackRequestPromise).request()); | ||
|
|
||
| expect(feedbackEvent.contexts?.trace?.trace_id).toMatch(/\w{32}/); | ||
| expect(feedbackEvent.contexts?.trace?.span_id).toMatch(/\w{16}/); |
There was a problem hiding this comment.
Test uses weak unanchored regex for trace assertions
Low Severity
The assertions for trace_id and span_id use /\w{32}/ and /\w{16}/ — unanchored regexes with \w (which matches underscores and uppercase letters). This is inconsistent with the stricter /^[a-f\d]{32}$/ used on line 62 in the same test. Since these are the core assertions validating the fix (that feedback events get proper trace context), they don't thoroughly verify the newly added behavior. A string longer than 32 characters or containing non-hex characters would still pass.
Triggered by project rule: PR Review Guidelines for Cursor Bot
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| evt.contexts = { | ||
| trace: getTraceContextFromScope(currentScope), | ||
| trace: { ...evt.contexts?.trace, ...getTraceContextFromScope(currentScope) }, | ||
| ...evt.contexts, | ||
| }; |
There was a problem hiding this comment.
Bug: The trace property in evt.contexts is overwritten by the object spread that follows it, preventing the scope's trace context from being correctly merged.
Severity: CRITICAL
Suggested Fix
To ensure the merged trace context is preserved, move the trace property definition after the spread of evt.contexts. The correct implementation should be: evt.contexts = { ...evt.contexts, trace: { ...evt.contexts?.trace, ...getTraceContextFromScope(currentScope) } };
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: packages/core/src/client.ts#L1224-L1227
Potential issue: In `_prepareEvent`, the code attempts to merge trace context from the
scope into an event's contexts. However, due to the property order in the object
literal, the `trace` property is defined and then immediately overwritten by the spread
of the original `evt.contexts`. The line `...evt.contexts` comes after the new `trace`
property, causing the original `evt.contexts.trace` to replace the newly merged one.
This negates the intended merge, preventing trace information from the scope from being
added to events that already have a trace context, such as user feedback events.
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
|
|
||
| evt.contexts = { | ||
| trace: getTraceContextFromScope(currentScope), | ||
| trace: { ...evt.contexts?.trace, ...getTraceContextFromScope(currentScope) }, |
There was a problem hiding this comment.
Trace context merge is overwritten by spread order
Medium Severity
The computed merge { ...evt.contexts?.trace, ...getTraceContextFromScope(currentScope) } on line 1225 is immediately overwritten by ...evt.contexts on line 1226, because later properties in an object literal override earlier ones with the same key. When evt.contexts has a trace property, the spread replaces the merged trace with the original unmerged value, making this change a no-op compared to the old code. If the intent was to ensure scope trace data supplements existing trace context, trace needs to appear after ...evt.contexts in the object literal.
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
2 participants
Closes #19681 (added automatically)