Skipped Reporting of 302 and Request abort error #4881

SukhvirKooner · 2025-01-28T17:24:36Z

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:

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

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

Added a central AbortController to cancel pending requests

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

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

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

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.

Review guidance

1. Rebuild with PR changes

Pull this branch, restart the dev server.

2. Add debug logs

Cancellation branch at the top of the ignore‑conditional:

if (isCancel(error) || ) {
  console.log('[DEBUG] request canceled or ignored:', error.message);
  return Promise.reject(error);
}

Sentry branch at the top of the production-reporting block:

} else {
  console.log('[SENTRY] WOULD REPORT:', { message, url, status });
  Sentry.withScope();
}

Expose the controller for manual aborts:

// after `let abortController = new AbortController()`
window._axiosAbortController = abortController;

Test request abort suppression

In the app, trigger an Axios request (e.g., navigate to a published channel page).
Open DevTools → Console and run:

window._axiosAbortController.abort();

Expect to see only:

[DEBUG] request canceled or ignored: canceled

No [SENTRY] WOULD REPORT: line should appear.

This confirms that manually aborted requests are successfully caught and suppressed before they can be reported to Sentry. By logging only [DEBUG] request canceled or ignored and not triggering the [SENTRY] WOULD REPORT: log

akolson · 2025-02-03T15:16:15Z

contentcuration/contentcuration/frontend/shared/client.js

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


Is this being used anywhere?

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.

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?

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

akolson · 2025-02-03T15:35:32Z

contentcuration/contentcuration/frontend/shared/client.js

+  // Reset the flag after navigation is likely complete
+  setTimeout(() => {
+    isNavigating = false;
+  }, 100);


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?

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 · 2025-02-04T14:15:17Z

contentcuration/contentcuration/frontend/shared/client.js

+    if (
+      isNavigating ||
+      isReloading ||
+      axios.isCancel(error) ||


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) ||

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

contentcuration/contentcuration/frontend/shared/client.js

SukhvirKooner · 2025-02-05T16:34:59Z

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 · 2025-02-20T14:03:18Z

@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 · 2025-02-20T17:42:59Z

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 · 2025-02-21T20:38:33Z

contentcuration/contentcuration/frontend/shared/client.js

+  const newAbortController = new AbortController();
+  abortController.signal = newAbortController.signal;
+
+  // Listen for the abort event to reset the navigation flag
+  newAbortController.signal.addEventListener('abort', () => {
+    isNavigating = false;
+  });
+
+  // Abort pending requests when navigation begins
+  abortController.abort();


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

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 · 2025-02-25T12:55:30Z

contentcuration/contentcuration/frontend/shared/client.js

@@ -82,6 +129,8 @@ client.interceptors.response.use(
            Network: {
              lastOffline: lastOffline ? `${Date.now() - lastOffline}ms ago` : 'never',
              online: navigator.onLine,
+              isNavigating,
+              pendingRequestCount: pendingRequests.size,


is there any significance in reporting this to sentry?

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.

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

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?

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.

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 · 2025-03-12T09:21:45Z

Hi @SukhvirKooner! Any progress on this issue?

SukhvirKooner · 2025-03-12T09:59:23Z

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 · 2025-03-13T20:29:12Z

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 · 2025-04-14T06:28:19Z

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 · 2025-04-14T11:15:28Z

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

SukhvirKooner · 2025-04-14T20:30:24Z

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 · 2025-04-15T11:16:22Z

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?

SukhvirKooner · 2025-04-16T16:50:02Z

Hi @akolson
I’ve completed the rebase. I'll add a detailed "review guidance" section with manual test instructions shortly.

SukhvirKooner · 2025-04-18T19:18:52Z

Hi @akolson ,
I have added a detailed Review guidance section with step-by-step manual test instructions to help verify that the request abort error is properly suppressed and no longer reported to Sentry.

akolson · 2025-04-22T11:02:39Z

@coderabbitai full review

akolson · 2025-04-22T11:04:43Z

contentcuration/contentcuration/frontend/shared/client.js

+  const newAbortController = new AbortController();
+  abortController.signal = newAbortController.signal;
+
+  // Abort pending requests when navigation begins


Lets remove this comment. The code is self explanatory

akolson · 2025-04-22T11:04:55Z

contentcuration/contentcuration/frontend/shared/client.js

+  abortController.abort();
+});
+
+// Add request interceptor to track pending requests


https://github.com/learningequality/studio/pull/4881/files#r2053882264

akolson · 2025-04-22T11:05:24Z

contentcuration/contentcuration/frontend/shared/client.js

+
+// Add request interceptor to track pending requests
+client.interceptors.request.use(config => {
+  // Add signal to the request config


https://github.com/learningequality/studio/pull/4881/files#r2053882264

akolson · 2025-04-22T11:05:55Z

contentcuration/contentcuration/frontend/shared/client.js

@@ -38,24 +38,59 @@ window.addEventListener('offline', () => {
  lastOffline = Date.now();
 });

+const pendingRequests = new Set();
+
+// Create an AbortController for managing pending requests


https://github.com/learningequality/studio/pull/4881/files#r2053882264

akolson · 2025-04-22T11:06:34Z

contentcuration/contentcuration/frontend/shared/client.js

+      pendingRequests.delete(error.config);
+    }
+
+    // Ignore specific types of errors


https://github.com/learningequality/studio/pull/4881/files#r2053882264

akolson · 2025-04-22T11:06:41Z

contentcuration/contentcuration/frontend/shared/client.js

+    // Ignore specific types of errors
+    if (
+      isCancel(error) ||
+      (error.response && [302, 403, 404, 405, 412].includes(error.response.status)) // added 302


https://github.com/learningequality/studio/pull/4881/files#r2053882264

SukhvirKooner · 2025-04-22T11:18:06Z

I have removed all the mentioned comments. Please let me know if any others still need to be removed.

akolson · 2025-04-22T11:18:46Z

contentcuration/contentcuration/frontend/shared/client.js

 client.interceptors.response.use(
-  response => response,
+  response => {
+    // Remove from pending requests on success


Lets remove this too!

contentcuration/contentcuration/frontend/shared/client.js

akolson · 2025-04-22T11:21:08Z

contentcuration/contentcuration/frontend/shared/client.js

+    const status = error.response?.status || 0;
+
+    // Suppress contentnode query errors
+    if (url.includes('contentnode')) {


Do we know status error code the affected url returns?

aborted requests never actually hit the server, so there is no HTTP status code returned. In the Network tab you’ll just see (canceled), and in our interceptor error.response will be undefined. Axios then falls back to status = 0 and throws a CanceledError with error.code === 'ERR_CANCELED'.

That’s exactly why our suppression logic uses:

if (isCancel(error)) { return Promise.reject(error); }

rather than checking for a particular HTTP response code. Any request you manually abort (or that gets aborted by navigation) will show up as status 0 or ERR_CANCELED, so it’s correctly caught by isCancel(error) and never forwarded to Sentry.

Agreed 💯.
So do we still need to specifically do if (url.includes('contentnode')) { when if (isCancel(error)) { is doing this for us?

Yes – we still need both guards, because they cover different noise:

isCancel(error) catches aborted requests (status 0, ERR_CANCELED)

url.includes('contentnode') silences real HTTP errors from our content‑node API that are "safe to ignore"

Removing the contentnode check may flood Sentry with expected CMS glitches, removing isCancel would report every navigation‑cancelled request. We need them both.

But if u think that's still not needed , I'll be happy to remove it.

I actually think it is the same thing based on your explanation 🙂. I think we should remove this check. I also think it is a rigid check -- it would fail for other APIs that don't contain the contentnode path with the same failure scenario.

We can always add this back if sentry says otherwise

Totally fair, makes sense to remove it for now. we can always revisit.

akolson · 2025-04-22T11:26:45Z

Hi @SukhvirKooner!
Thanks for the reviewer guidance. However, this might be a little difficult to test especially for the QA team (for dev, no problem at all), if we need to manipulate/update the code in the pr.. Don't we have a more straight forward way to test this? (I'm proceeding to test this with the above. Just some food for thought). I also have a question here

SukhvirKooner · 2025-04-22T11:57:17Z

Thanks! This was the approach I came up with to test that the request abort suppression is working as expected. I’ll also look into whether there’s a way to test this without making any changes in the PR code itself.

akolson

LGTM!
Thanks and Great work @SukhvirKooner. Sukhvir added some reviewer guidance though I think it it more developer focused. The most effective way for QA to test this is to check the sentry reports (after going through several workflows) to see if any of the suppressed errors show up there again. If they don't we can assume the issue has been resolved. cc @radinamatic @pcenov

MisRob requested review from akolson, nucleogenesis, AlexVelezLl and ozer550 January 28, 2025 19:16

akolson reviewed Feb 3, 2025

View reviewed changes

akolson reviewed Feb 4, 2025

View reviewed changes

contentcuration/contentcuration/frontend/shared/client.js Outdated Show resolved Hide resolved

akolson reviewed Feb 6, 2025

View reviewed changes

rtibbles assigned akolson Feb 11, 2025

SukhvirKooner force-pushed the issue_4873 branch from 67a3f38 to 01ee70e Compare February 20, 2025 17:40

nucleogenesis reviewed Feb 21, 2025

View reviewed changes

akolson reviewed Feb 25, 2025

View reviewed changes

MisRob requested review from akolson and nucleogenesis March 19, 2025 04:41

SukhvirKooner and others added 5 commits April 16, 2025 22:03

skipped reporting of 302 and request abort error

0ee2215

[pre-commit.ci lite] apply automatic fixes

50713f3

Remove unused function

694cd76

[pre-commit.ci lite] apply automatic fixes

53048fa

lint error fix

cd66673

SukhvirKooner added 3 commits April 16, 2025 22:03

added comments

81c7653

clean up Sentry reports

fd6b7ea

remove unnecessary error checks

40ec9a7

SukhvirKooner force-pushed the issue_4873 branch from 8a5e9e0 to 40ec9a7 Compare April 16, 2025 16:34

[pre-commit.ci lite] apply automatic fixes

7c363a5

akolson reviewed Apr 22, 2025

View reviewed changes

removed unnecessary comments

1fa90fa

akolson reviewed Apr 22, 2025

View reviewed changes

contentcuration/contentcuration/frontend/shared/client.js Outdated Show resolved Hide resolved

removed unnecessary comments

b93f075

akolson reviewed Apr 22, 2025

View reviewed changes

removed contentnode error suppression check

0fa7a67

akolson approved these changes Apr 22, 2025

View reviewed changes

akolson merged commit 26005dc into learningequality:unstable Apr 22, 2025
13 checks passed

Skipped Reporting of 302 and Request abort error #4881

Skipped Reporting of 302 and Request abort error #4881

Uh oh!

Conversation

SukhvirKooner commented Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Redirect requests (301/302)

Request aborted Error

Review guidance

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SukhvirKooner commented Feb 5, 2025

Uh oh!

akolson left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akolson commented Feb 20, 2025

Uh oh!

SukhvirKooner commented Feb 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akolson commented Mar 12, 2025

Uh oh!

SukhvirKooner commented Mar 12, 2025

Uh oh!

SukhvirKooner commented Mar 13, 2025

Uh oh!

SukhvirKooner commented Apr 14, 2025

Uh oh!

akolson commented Apr 14, 2025

Uh oh!

SukhvirKooner commented Apr 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

akolson commented Apr 15, 2025

Uh oh!

SukhvirKooner commented Apr 16, 2025

Uh oh!

SukhvirKooner commented Apr 18, 2025

Uh oh!

akolson commented Apr 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

SukhvirKooner commented Jan 28, 2025 •

edited

Loading

akolson left a comment •

edited

Loading

SukhvirKooner commented Apr 14, 2025 •

edited

Loading