Skip to content

Conversation

@kinyoklion
Copy link
Member

@kinyoklion kinyoklion commented Feb 27, 2025

Resolves a bug where the srcLine and before/after context would have been populated incorrectly.

We would assume the src line would be in the middle of the source context, but when a number of lines greater than the context window don't exist, it will not always be in the middle.

The fix extends TraceKit with a srcStart field that indicates the line the context window starts on. This can then be used to calculate the offset to the originating line.

@github-actions
Copy link
Contributor

@launchdarkly/js-sdk-common size report
This is the brotli compressed size of the ESM build.
Size: 19014 bytes
Size limit: 21000

@github-actions
Copy link
Contributor

@launchdarkly/js-client-sdk-common size report
This is the brotli compressed size of the ESM build.
Size: 15371 bytes
Size limit: 20000

function: inFrame.func,
line: inFrame.line,
col: inFrame.column,
// Strip the nulls so we only ever return undefined.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is from tightening up the typing a bit in the vendored library.

@github-actions
Copy link
Contributor

@launchdarkly/js-client-sdk size report
This is the brotli compressed size of the ESM build.
Size: 19871 bytes
Size limit: 21000

@kinyoklion kinyoklion force-pushed the rlamb/emsr-155/fix-src-context-bug branch 2 times, most recently from 5840a4a to a73f4ae Compare February 28, 2025 00:26
@kinyoklion kinyoklion marked this pull request as ready for review February 28, 2025 00:26
@kinyoklion kinyoklion requested a review from a team as a code owner February 28, 2025 00:26

element.context = element.line ? gatherContext(element.url, element.line) : null;
const srcContext = gatherContext(element.url, element.line!);
element.context = element.line ? (srcContext?.contextLines ?? null) : null;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The typing of some of this stuff is pretty strange, but I am not trying to comprehensively re-work the existing typing. So keeping it basically the same for now.

@kinyoklion kinyoklion force-pushed the rlamb/emsr-155/fix-src-context-bug branch from a73f4ae to 0b7001c Compare February 28, 2025 00:33
'}',
'foo();',
],
srcStart: 1,
Copy link
Member Author

@kinyoklion kinyoklion Feb 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

srcStart is 1-based and is 1, line is 1-based and 3.
3 - 1 = 2; Now we use the offset from the start for the src line.
Index 0 == throw new Error("error on line 3");'

@kinyoklion kinyoklion requested a review from a team February 28, 2025 00:40
@kinyoklion kinyoklion merged commit 1f44dd5 into main Feb 28, 2025
24 checks passed
@kinyoklion kinyoklion deleted the rlamb/emsr-155/fix-src-context-bug branch February 28, 2025 16:43
@github-actions github-actions bot mentioned this pull request Feb 28, 2025
kinyoklion pushed a commit that referenced this pull request Feb 28, 2025
🤖 I have created a release *beep* *boop*
---


<details><summary>browser-telemetry: 1.0.2</summary>

##
[1.0.2](browser-telemetry-v1.0.1...browser-telemetry-v1.0.2)
(2025-02-28)


### Bug Fixes

* Fix a bug where the incorrect src lines may have been captured.
([#792](#792))
([1f44dd5](1f44dd5))
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants