Skip to content

Conversation

@miquelbeltran
Copy link
Contributor

Evaluates and continues work from #434

Codex- and others added 7 commits September 21, 2023 10:57
- Returning this promise allows the parent catch block to handle the promise rejection.
- The promise started handles its own exceptions.
- The call to self.executeHandlers is already wrapped with a try catch.
- executeHandlers has already been wrapped with wrapWithHandler.
- Allows `disableFetchLogging` to function.
- This gives the body being passed to the handlers more meaning.
@miquelbeltran miquelbeltran requested a review from Copilot June 30, 2025 07:51
@miquelbeltran miquelbeltran self-assigned this Jun 30, 2025
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR enhances network tracking by improving fetch interception to handle nested response.text() promises and by managing the original fetch reference.

  • Store the original fetch in this.originalFetch and expose a disableFetchLogging method for restoration.
  • Refactor fetch processing to clone responses, truncate text bodies, and handle promise rejections robustly.
  • Remove wrapWithHandler around XHR’s send, load, and error callbacks, using inline handlers instead.
Comments suppressed due to low confidence (2)

src/raygun.network-tracking.js:206

  • The newly added branch for handling response.text() fulfillment and rejection isn't covered by existing tests. Consider adding tests to exercise both the successful truncation and the rejection fallback paths.
            if (ourResponse && typeof ourResponse.text === 'function') {

src/raygun.network-tracking.js:250

  • The new disableFetchLogging is attached to NetworkTracking.prototype, but it's unclear if NetworkTracking is the correct constructor or imported identifier. Additionally, brace alignment appears off and may cause a syntax error. Verify the target object and fix the mismatched braces.
  NetworkTracking.prototype.disableFetchLogging = function() {

this,
'send',
self.wrapWithHandler(function() {
function() {
Copy link

Copilot AI Jun 30, 2025

Choose a reason for hiding this comment

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

Removing wrapWithHandler from the XHR send enhancement means exceptions in this inline handler will no longer be caught and logged. Consider wrapping this function with self.wrapWithHandler or adding an explicit try/catch to preserve error handling.

Suggested change
function() {
self.wrapWithHandler(function() {

Copilot uses AI. Check for mistakes.
this.addEventListener(
'load',
self.wrapWithHandler(function() {
function() {
Copy link

Copilot AI Jun 30, 2025

Choose a reason for hiding this comment

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

The load event listener is now an inline function without the previous wrapWithHandler guard. If this callback throws, errors will be uncaught. Please reintroduce wrapWithHandler or a try/catch block here.

Suggested change
function() {
self.wrapWithHandler(function() {

Copilot uses AI. Check for mistakes.
Comment on lines +124 to +130
function() {
self.executeHandlers(self.errorHandlers, {
requestURL: url,
responseURL: this.responseURL,
duration: new Date().getTime() - initTime,
});
})
}
Copy link

Copilot AI Jun 30, 2025

Choose a reason for hiding this comment

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

Similarly, the error event listener no longer uses wrapWithHandler, which may lead to uncaught exceptions. Consider restoring the wrapper or adding error handling locally.

Copilot uses AI. Check for mistakes.
duration: new Date().getTime() - initTime,
},
promise
.then(function (response) {
Copy link

Copilot AI Jun 30, 2025

Choose a reason for hiding this comment

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

[nitpick] The nested promise chain for fetch response handling can be hard to follow. Refactoring this logic into an async function with await could improve readability and reduce nesting.

Copilot uses AI. Check for mistakes.
@miquelbeltran
Copy link
Contributor Author

Looking at the original PR, it is removing the calls to wrapWithHandler.

This function is wrapping any call in a try/catch and then logging the error into Raygun. That works to catch any errors in our code, however if a customer needs that original exception to be passed up the fetch method, it would be lost.

    this.wrapWithHandler = function(method) {
      return function() {
        try {
          return method.apply(this, arguments);
        } catch (ex) {
          Raygun.Utilities.log(ex);
        }
      };
    };

I think that an alternative solution is instead to throw the exception after capturing it, like so:

    this.wrapWithHandler = function(method) {
      return function() {
        try {
          return method.apply(this, arguments);
        } catch (ex) {
          Raygun.Utilities.log(ex);
+++       throw ex;
        }
      };
    };

@miquelbeltran miquelbeltran marked this pull request as draft June 30, 2025 11:45
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