Skipped Reporting of 302 and Request abort error

[SukhvirKooner]

Fixes: #4873

Summary

Redirect requests (301/302)

Before

After

I have successfully resolved the 302 redirect request issue by updating the Axios error handling logic. Specifically, I added 302 to the list of status codes that should bypass error logging:
(error.response && [302, 403, 404, 405, 412].includes(error.response.status))
This ensures that when a 302 status is returned, Axios will handle it gracefully without triggering unnecessary error logs or forwarding it to Sentry.

Request aborted Error

Error ScreenShort

I have made the key changes to the original code to handle the Error particular during navigation:

1. Page State Tracking:

let isNavigating = false;
let isReloading = false;
let pendingRequests = new Set();

• Added variables to track navigation state and keep track of pending requests
• This helps manage network requests during page transitions

2. Abort Controller Integration:
   const abortController = new AbortController();

• Added a central AbortController to cancel pending requests

3. Navigation Event Handling:

window.addEventListener('navigate', () => {
  isNavigating = true;
  abortController.abort();
  setTimeout(() => {
    isNavigating = false;
  }, 100);
});

window.addEventListener('beforeunload', () => {
  abortController.abort();
});

• Listens for navigation events and aborts pending requests
• Uses a timeout to reset the navigation state
• Handles page unload events

4. Request Tracking in Interceptors:

client.interceptors.request.use(config => {
  config.signal = abortController.signal;
  pendingRequests.add(config);
  return config;
});

• Adds abort signal to each request
• Tracks pending requests in the Set

5. Enhanced Error Handling:

if (
  isNavigating || 
  isReloading ||
  axios.isCancel(error) ||
  error.code === 'ERR_CANCELED' ||
  error.code === 'ECONNABORTED' ||
  (error.response && [302, 403, 404, 405, 412].includes(error.response.status))
) {
  return Promise.reject(error);
}

• Added comprehensive error checking
• Silently handles navigation-related cancellations and specific HTTP status codes
• Added 302 status code to the suppressed errors list

6. Cleanup Function:

export function cleanupRequests() {
  abortController.abort();
  pendingRequests.clear();
}

• Added function to clean up pending requests
• Useful when components unmount

Despite these suppression mechanisms, legitimate errors unrelated to Request aborted or Redirect requests (301/302) will still be logged and captured in Sentry for further monitoring and troubleshooting.

[akolson]

Is this being used anywhere?

[SukhvirKooner]

The exported cleanupRequests function is meant to be called when a component unmounts. It aborts any pending requests and clears the pendingRequests set to prevent memory leaks or unwanted side effects.

[akolson]

Right! Unfortunately I don't see any usages of this function. Makes me wonder whether we really need it, particularly within the scope of this task?

[SukhvirKooner]

Got it! I'll go ahead and remove it.

[akolson]

I don't think the use of a delay here is a reliable way to set the navigation to false. I think it would be better to do this after an abort has been confirmed. You could consider adding an event listener to an controller.signal instance and do the setting there?

[SukhvirKooner]

You're absolutely right—relying on a fixed delay is not a reliable way to reset the navigation flag. Using an event listener on the AbortController.signal makes a lot more sense since it ensures the state update happens precisely when the abort is confirmed. I'll update as you suggested.

[akolson]

Let's import the isCancel instead so we can clear the linting error below ie;
import {isCancel} from 'axios' and this line changes to isCancel(error) ||

[SukhvirKooner]

Thank you for the suggestion! I'll make sure to update it accordingly.

[akolson]

Not sure about the motivation for this. The issue description doesn't seem to suggest this.

[SukhvirKooner]

• The primary goal here is to suppress errors specifically related to content nodes, as they are a significant source of the issues we need to address.
• Since the URLs containing the keyword contentnode reliably identify these errors, this condition becomes essential for targeted error suppression.
• I acknowledge that this detail should have been explicitly mentioned in the issue description.

[akolson]

Qn: What happens in the event that we receive requests without the "contentnode"? Wouldn't it make more sense to block out all XHR requests all together and not target specifics?

[SukhvirKooner]

That's a valid concern. The reasoning behind targeting only contentnode requests is to avoid suppressing errors that might be relevant for debugging other parts of the application. Broadly blocking all XHR requests could potentially hide critical issues.

[akolson]

I think the blocking is only applicable to the 302 errors, right?

[SukhvirKooner]

Hi @akolson

• Thanks for sharing the Sentry logs—this definitely gives me more clarity on the errors that need suppression. I noticed that all the aborted requests are marked with [0].
• Do you think all [0] code errors should be suppressed, or are there specific ones we should target?
• Also, do you have any recommendation or best practices for suppressing all such errors efficiently? Would love to hear your thoughts on this.

[akolson]

Hi @SukhvirKooner! Apologies for not getting back soon enough I was OOO.

Do you think all [0] code errors should be suppressed, or are there specific ones we should target?

I think it would be great to establish if ERR_CANCELED and ECONNABORTED states translate to any numerical equivalent first. If not, I think checking for them directly should suffice and is more explicit. Based on sentry, it appears that these kinds of errors are associated with http error code "0". We can check the axios code to see if we can find anything.

Also, do you have any recommendation or best practices for suppressing all such errors efficiently? Would love to hear your thoughts on this.

I think your general approach good. But so we are clear, there is nothing wrong with your approach. What we are doing here is ironing out any kinks so the implementation is more clear and robust.

1. We don't need to explicitly check for both ERR_CANCELED and ECONNABORTED codes. Sticking with isCancel is probably sufficient. Based on the axios code, both the cancel and abort actions update the same property(__CANCEL__) which isCancel checks, so generally safe to use it unless our intention is to narrow down to a specific error.
2. it's probably overkill to manually cancel pending requests on navigation as Axios handles this for us automatically after certain criteria is met, right? Axios automatically kills off unresponsive, hanging requests after a certain timeout is hit.
3. And this now begs the question whether there is any significance in tracking navigation? Maybe we should be focusing on suppressing the errors and not cancelling them (that's the impression I get from the issue, but I could be wrong)?

Be sure to let us know in case something is not clear or you need more guidance. Thanks again for for working on this issue persistently. This is much appreciated.

[SukhvirKooner]

Hi @akolson,

Thanks for the detailed explanation! That makes a lot of sense. I’ll go ahead and update the implementation to use isCancel instead of explicitly checking for ERR_CANCELED and ECONNABORTED. Also, I’ll remove the manual cancellation on navigation since Axios already handles it, and then I'll verify that the errors are still being suppressed as expected.

I’ll focus on suppressing these errors rather than tracking navigation unless we find a specific need for it. Appreciate your insights.

[SukhvirKooner]

I think it would be great to establish if ERR_CANCELED and ECONNABORTED states translate to any numerical equivalent first. If not, I think checking for them directly should suffice and is more explicit. Based on sentry, it appears that these kinds of errors are associated with http error code "0". We can check the axios code to see if we can find anything.

Axios assigns these string codes in its internal error handling. For example:

• error.code === 'ERR_CANCELED' indicates a canceled request (often via CancelToken or the new AbortController).

• error.code === 'ECONNABORTED' usually indicates a timeout or a similar connection abort scenario.

They are not standard HTTP status codes, so they typically show up with error.response?.status of 0 or undefined—which is why they’re logged as code 0 in Sentry and undefined in web browser.

[akolson]

Great. Thanks for investigating this @SukhvirKooner. Hopefully these insights help with the changes to be made. Thank you!

[SukhvirKooner]

Hi @akolson ,
I have removed the unused functions and updated the code as suggested.

[akolson]

Changes seem to make sense. Thanks @SukhvirKooner. I'll invite @nucleogenesis @AlexVelezLl @ozer550 incase of any further feedback before we proceed with the approval

[akolson]

@SukhvirKooner It looks like the linting check is failing, could you please look into this?

Also, @nucleogenesis @AlexVelezLl @ozer550, any additional thoughts on this?

[SukhvirKooner]

Hi @akolson,
Thank you for pointing this out. The linting issue has been resolved. Please let me know if there’s anything else you’d like me to review.

[nucleogenesis]

Why do we need a new abort controller here? I'm not super familiar w/ AbortController, but how I read this is that it creates newAbortController, then sets the abortController.signal to the default value of a newly created AbortController instance. Then you put the event listener on that signal using the newAbortController (which now also sets the listener on the original abortController.signal unless I'm mistaken) - and then you call the abort() on the original controller.

So I'm wondering if this was done because you need a fresh AbortSignal value assigned to the original abortController value created above for some reason?

If it works it works, but perhaps a comment explaining why we need this temporary abort controller object for it's signal value alone would be helpful for anybody looking at this in the future

[SukhvirKooner]

You're right, The reason for creating a new AbortController and assigning its signal to the original one is to ensure that each navigation event gets a fresh AbortSignal. This allows proper cancellation of only the relevant pending requests tied to that specific navigation.

Without a fresh signal, previously aborted requests might interfere with new requests triggered by subsequent navigations. The fresh AbortSignal ensures each navigation cycle has its own independent abort handling.

I'll add a comment in the code to clarify this for future reference. Thanks again for pointing it out!

[akolson]

is there any significance in reporting this to sentry?

[SukhvirKooner]

Reporting isNavigating and pendingRequestCount to Sentry could be useful for debugging network-related issues:

• isNavigating: Helps correlate errors with page transitions, potentially revealing issues triggered during navigation.
• pendingRequestCount: Shows the number of pending requests when an error occurs, assisting in identifying performance bottlenecks or race conditions.

If there are no related issues currently, we can skip this. However, it might be valuable for future debugging.

[akolson]

I am not entirely sure there is a lot to deduce from these two pieces of information when sent to sentry

[SukhvirKooner]

That's a fair point. If these values alone don’t provide enough insight, we could consider pairing them with additional context like error codes to make them more meaningful in Sentry. Thoughts?

[akolson]

I think error codes would not make much of a difference. My point is this; we don't need to bloat the sentry reports especially if we are not getting much benefit from the data we are trying to report.

[SukhvirKooner]

I understand your point, and I’ll remove this right away to keep the Sentry reports concise and relevant. Let me know if there’s anything else you’d like me to adjust.

[akolson]

Hi @SukhvirKooner! Any progress on this issue?

[SukhvirKooner]

Hi @SukhvirKooner! Any progress on this issue?

Hi @akolson
Yes, I am working on it. I’ve removed the explicit checks for ERR_CANCELED and ECONNABORTED, as well as the manual cancellation on navigation. Currently, I’m testing to ensure that the error doesn’t get reported. I’ll update the PR shortly.

[SukhvirKooner]

Hi @akolson
I have successfully removed the redundant checks for isNavigating, error.code === 'ERR_CANCELED', and error.code === 'ECONNABORTED'. The isCancel(error) function already covers all cancellation scenarios, including those triggered by navigation events or manual aborts. This simplifies the error handling logic and reduces unnecessary conditions in the code.

Looking forward to your review and getting this merged!

[SukhvirKooner]

Hi,
just checking in to see if there’s been any update on this PR? Let me know if there’s anything needed from my end to help move it forward. Thanks!

[akolson]

Hi @SukhvirKooner! Could you please rebase this with the latest changes in unstable?

[SukhvirKooner]

Hi @akolson, I just checked and there haven’t been any updates to the client.js file in the recent PRs to unstable.
Could you let me know if there’s something specific you would like me to rebase or look into?

[akolson]

Hi @SukhvirKooner ! The goal of the rebase is to keep your branch up to date. Because it's stayed out of unstable for quite a while now (our apologies for that), we are just making sure that all the changes from unstable are brought into your branch and also want to avoid any conflicts that could occur. You might need to force push the changes in your pr.

Also, could you please outline clear steps on how to manually test your PR--unless I missed them, It didn't get the sense that they were included. Could you please be more explicit by adding a "review guidance" section?