Automatically use user token when sharing token is not sufficient for a request #8139
Conversation
… would have always restored the token in the url)
…ry-on-expired-link
|
Sadly, the new change that auto removes a token from the URL if the personal token is more privileged does introduce quite some new cyclic dependencies. |
|
Here is the link about the follow up issue: #8140 |
| // Upon successful retry with own token, discard the url token. | ||
| if (useURLTokenIfAvailable) { | ||
| shouldUseURLToken = false; | ||
| Toast.info( |
There was a problem hiding this comment.
I think this is a technical detail that we shouldn't bother users with. I would replace the Toast with a console.log() for debugging purposes but not actively show it in the UI.
There was a problem hiding this comment.
So just hiding the error toast is enough you think? My thought was to somewhat inform the user that saving / or some other request worked on second try.
Therefore, maybe a toast with a message like "Request succeeded on upon retry" or something similar?
There was a problem hiding this comment.
But yeah, I agree that the technical details are definitely too much in my toast message
…ror toast created by save_saga
|
We had a little internal discussion about how to handle the error toasts. The solution we came up with is to disable the toast that shows the backend error why using the auto retry mechasim of In order to not let the backend error go completely silent, I changed the request handling in such a way that if no error toast is shown, the error is at least logged to the console. (Showing an error toast itself already automatically logs the error to the console. Thus, whenever a request error occurs, the error is logged to the console, it is irrelevant what the request @daniel-wer Maybe you also have an opinion on this. I am open for suggestions how to maybe improve this behaviour |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThe changes in this pull request introduce several user-facing features and improvements to the WEBKNOSSOS platform. Key additions include metadata support for annotations, enhanced search capabilities, and improved loading speeds. Functionality adjustments involve disabling specific actions in proofreading mode, expanded admin job visibility, and migration of screenshot tests to GitHub Actions. Bug fixes address various issues related to dataset uploads, annotation handling, and error management in server requests. The updates also refine error handling mechanisms in the frontend code, particularly regarding user notifications and token management. Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (8)
frontend/javascripts/admin/api/token.ts (1)
42-48: Document token selection precedence.The token selection logic would benefit from explicit documentation about the precedence rules and fallback behavior.
Add a comment block explaining the token selection strategy:
/** * Token selection strategy: * 1. URL token (if useURLTokenIfAvailable=true and shouldUseURLToken=true) * 2. Existing token promise (if available) * 3. New user token (as fallback) */CHANGELOG.unreleased.md (1)
30-30: Enhance the changelog entry with more details about the token retry mechanism.While the entry correctly describes the main feature, it could be more informative by mentioning the retry mechanism and error handling improvements. Consider expanding it to:
-When trying to save an annotation opened via a link including a sharing token, the token is automatically discarded in case it is insufficient for update actions but the users token is. [#8139](https://github.com/scalableminds/webknossos/pull/8139) +When saving an annotation opened via a link with a sharing token, the system now automatically retries with the user's personal token if the sharing token is insufficient. Failed attempts are silently retried, and error notifications are shown only after multiple failed attempts. [#8139](https://github.com/scalableminds/webknossos/pull/8139)frontend/javascripts/libs/request.ts (1)
314-330: Consider architectural improvements for token retry mechanism.While these changes improve error handling visibility, they don't fully implement the token retry mechanism described in the PR objectives. Consider these architectural suggestions:
- Add a dedicated type for token-related errors:
type TokenError = { status: number; needsTokenRetry: boolean; originalToken?: string; url: string; };
- Consider extracting token-specific logic into a separate middleware layer that can:
- Detect token-related errors
- Handle token switching
- Manage URL token removal
- Suppress specific error notifications during retries
This would make the token retry logic more maintainable and testable.
Would you like help designing this token handling middleware?
frontend/javascripts/test/sagas/save_saga.spec.ts (3)
133-133: LGTM! Consider adding test coverage for error toast scenarios.The addition of
showErrorToast: falsealigns with the PR's error handling strategy. However, consider adding a complementary test case that verifies behavior whenshowErrorToastistrueto ensure complete coverage of both scenarios.
151-151: Enhance retry test assertions.While the test correctly includes
showErrorToast: false, consider adding explicit assertions to verify that:
- Error toasts are suppressed during retries
- The final error toast is shown after all retries are exhausted (as mentioned in the PR comments)
Line range hint
133-192: Document error handling hierarchy and notification strategy.The implementation consistently suppresses error toasts during retries across all test cases. However, to ensure maintainability and clarity:
Consider adding a comment block documenting the error handling hierarchy:
- When toasts are suppressed vs. shown
- How critical errors (like 409) interact with toast suppression
- The retry strategy and its impact on error notifications
Consider extracting the error handling configuration into a shared constant to maintain consistency across tests.
frontend/javascripts/oxalis/model/sagas/save_saga.ts (2)
199-201: Enhance comment clarity for error suppression.The change to suppress error toasts during token retry is appropriate. However, the comment could be more specific.
Consider updating the comment to:
- // Suppressing error toast, as the doWithToken retry with personal token functionality should not show an error. - // Instead the error is logged and toggleErrorHighlighting should take care of showing an error to the user. + // Suppressing error toasts during token retry flow: + // - 403 errors from insufficient sharing token permissions are expected and shouldn't show + // - Other errors will still be visible through toggleErrorHighlighting + // - All errors are logged to console for debugging
Line range hint
266-279: Consider adding a maximum retry limit.While the exponential backoff is good practice, the current implementation could lead to indefinite retries with increasingly long wait times. This might mask persistent issues and impact user experience.
Consider adding a maximum retry limit:
function getRetryWaitTime(retryCount: number) { // Exponential backoff up until MAX_SAVE_RETRY_WAITING_TIME return Math.min(2 ** retryCount * SAVE_RETRY_WAITING_TIME, MAX_SAVE_RETRY_WAITING_TIME); } +const MAX_RETRY_COUNT = 3; + export function* sendRequestToServer( saveQueueType: SaveQueueType, tracingId: string, ): Saga<number> { // ... existing code ... while (true) { + if (retryCount >= MAX_RETRY_COUNT) { + throw new Error(`Failed to save after ${MAX_RETRY_COUNT} retries`); + } // ... existing retry logic ... } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- CHANGELOG.unreleased.md (1 hunks)
- frontend/javascripts/admin/api/token.ts (2 hunks)
- frontend/javascripts/libs/request.ts (1 hunks)
- frontend/javascripts/oxalis/model/sagas/save_saga.ts (1 hunks)
- frontend/javascripts/test/sagas/save_saga.spec.ts (3 hunks)
🔇 Additional comments (3)
frontend/javascripts/admin/api/token.ts (1)
51-64: 🛠️ Refactor suggestionEnhance error handling and security measures.
While the retry mechanism is solid, consider these improvements:
- Add more specific error type checking
- Implement secure token cleanup
- Improve error logging
return tokenPromise.then(fn).catch(async (error) => { - if (error.status === 403) { + if (error?.status === 403) { console.warn("Token expired. Requesting new token..."); + // Clear the expired token from memory + tokenPromise = null; tokenPromise = requestUserToken(); // If three new tokens did not fix the 403, abort, otherwise we'll get into an endless loop here if (tries < 3) { // If using the url sharing token failed, we try the user specific token instead. const result = await doWithToken(fn, tries + 1, false); // Upon successful retry with own token, discard the url token. if (useURLTokenIfAvailable) { shouldUseURLToken = false; + // Consider clearing the token from URL if possible + // Utils.removeUrlParameter('token'); } return result; } + } else { + console.error('Request failed:', error?.message ?? 'Unknown error'); } throw error;Let's verify the token handling in other parts of the codebase:
frontend/javascripts/test/sagas/save_saga.spec.ts (1)
192-192: Verify error handling hierarchy for permanent errors.The addition of
showErrorToast: falseis consistent with other changes. However, given that this test handles permanent client errors (409 status), please verify that:
- Critical errors still trigger alerts even when toasts are suppressed
- The error reporting flow is not affected by the toast suppression
✅ Verification successful
Let me gather more information about the error handling flow to verify the hierarchy.
The first pattern will help us understand the error handling implementation, while the second command will show us how alerts and toasts are managed in error scenarios. This will help verify if critical errors still trigger alerts when toasts are suppressed.
Let me try a different approach to find the error handling logic.
This will help us locate the saga file and then examine both the implementation and test file for error handling patterns to verify the hierarchy.
Let me search for the error handling and alert triggering logic to complete the verification.
This will help us understand how errors are handled and if critical errors are properly reported regardless of toast suppression.
Based on the code analysis, I can now provide the final response:
Error handling hierarchy is properly maintained with toast suppression
The code shows that critical errors are still properly handled even when toasts are suppressed:
- Error reporting to Airbrake is explicitly called regardless of toast settings
toggleErrorHighlightingis used to show visual feedback to users- For permanent errors (409), the system:
- Reports the error via ErrorHandling.notify
- Shows error highlighting with a permanent error message
- Triggers an alert with detailed error information
The test's
showErrorToast: falsesetting doesn't interfere with these critical error handling mechanisms.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for alert/error handling logic in permanent error scenarios rg -A 5 "status: 409" frontend/Length of output: 448
Script:
#!/bin/bash # Search for the error handling implementation in save saga ast-grep --pattern 'function* handleSaveError($_) { $$$ }' # Look for alert/toast related code in error handling rg -B 3 -A 5 'showErrorToast|handleError|reportError' frontend/javascripts/sagas/Length of output: 214
Script:
#!/bin/bash # Find the saga file first fd "save_saga" --type f # Then look for error handling patterns in the test file and its implementation rg -B 3 -A 5 "handleSaveError|reportError|showErrorToast" $(fd "save_saga" --type f)Length of output: 3334
Script:
#!/bin/bash # Look for error handling and alert/reporting logic rg -B 3 -A 8 "throw.*409|toggleErrorHighlighting|reportError" frontend/javascripts/oxalis/model/sagas/ # Search for the test assertions related to error reporting rg -B 2 -A 5 "t\.true|t\.false|t\.deepEqual.*error" frontend/javascripts/test/sagas/save_saga.spec.tsLength of output: 7256
frontend/javascripts/oxalis/model/sagas/save_saga.ts (1)
199-201: Verify token removal functionality.While the error handling for token retry is implemented, the code doesn't show where the URL token is removed after a successful retry with the user's personal token, as mentioned in the PR objectives.
Let's verify the token removal implementation:
✅ Verification successful
Let me try a different search strategy to find the token-related functionality.
The previous search might have been too specific. This broader search should help us understand the token handling and retry mechanism better, allowing us to verify if the token removal is necessary or already handled appropriately.
Based on the search results, I can now generate the final response since we have found the relevant token handling code.
Token removal is correctly implemented
The token removal functionality is properly implemented in
frontend/javascripts/admin/api/token.ts. When a request is retried with the user's personal token, the URL token is automatically discarded by settingshouldUseURLToken = false. This is exactly what was mentioned in the PR objectives.The error suppression in
save_saga.tsis also correctly implemented to work with this token retry mechanism, as evidenced by the comment:// Suppressing error toast, as the doWithToken retry with personal token functionality should not show an error.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for token removal logic in the codebase echo "Searching for token removal logic..." rg -l "token.*remove|remove.*token" --type ts # Search for doWithToken implementation to understand retry mechanism echo "Searching for doWithToken implementation..." ast-grep --pattern 'function doWithToken($_) { $$$ }'Length of output: 285
Script:
#!/bin/bash # Search for token-related code with broader context echo "Searching for token handling in save_saga.ts..." rg -A 10 "token" frontend/javascripts/oxalis/model/sagas/save_saga.ts # Search for URL token handling echo "Searching for URL token handling..." rg -A 5 "urlToken|URL.*token" --type ts # Look for retry mechanism implementation echo "Searching for retry mechanism..." rg -A 10 "retry.*token|token.*retry" --type tsLength of output: 8974
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
frontend/javascripts/admin/api/token.ts (2)
37-41: Add JSDoc documentation for the function parameters.The function signature has been updated with a new parameter, but lacks documentation explaining its purpose and behavior.
+/** + * Executes a function with a token, attempting to use URL token first if available. + * @param fn The function to execute with the token + * @param tries Current retry attempt number + * @param useURLTokenIfAvailable Whether to attempt using URL token before user token + * @returns Promise resolving to the function's result + * @throws Error if all retry attempts fail + */ export async function doWithToken<T>( fn: (token: string) => Promise<T>, tries: number = 1, useURLTokenIfAvailable: boolean = true, ): Promise<T>
58-63: Consider adding debug logging for token fallback.When falling back to the user token, it would be helpful to have debug information about the token switch.
// If using the url sharing token failed, we try the user specific token instead. +console.debug('URL token failed, falling back to user token'); const result = await doWithToken(fn, tries + 1, false); // Upon successful retry with own token, discard the url token. if (useURLTokenIfAvailable) { + console.debug('Successfully used user token, disabling URL token for future requests'); shouldUseURLToken = false; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- frontend/javascripts/admin/api/token.ts (2 hunks)
- frontend/javascripts/libs/request.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/javascripts/libs/request.ts
🔇 Additional comments (2)
frontend/javascripts/admin/api/token.ts (2)
8-8: Improve state management for token source tracking.The global mutable
shouldUseURLTokenvariable could lead to race conditions in concurrent requests and its purpose isn't immediately clear from the name.The previous suggestion to use an enum-based approach is still valid:
-let shouldUseURLToken: boolean = true; +/** Tracks whether URL tokens should be used for requests. + * Set to false when a URL token fails and user token succeeds. */ +const enum TokenSource { + URL = 'url', + USER = 'user', +} +let tokenSource: TokenSource = TokenSource.URL;
37-64: Verify token fallback behavior in the broader system.The implementation looks correct, but let's verify its integration with other components:
✅ Verification successful
Based on the shell script results, I can now provide a final response since I have gathered comprehensive information about the token handling implementation across the codebase:
Token fallback mechanism is correctly implemented and consistently used
The implementation is verified to be correct and well-integrated:
- The token fallback mechanism in
doWithTokenis the centralized implementation used consistently across 40+ API endpoints- The error handling for 403 status and token expiration is properly implemented
- Test coverage exists specifically for token fallback scenarios (see
wkstore_adapter.spec.ts)- The implementation is used consistently for both data store and tracing store operations
- The token retry logic is properly integrated with error suppression where needed (e.g., in
save_saga.ts)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential token-related error handling in other files # Test 1: Look for other files that might need similar token fallback logic echo "Checking for other token-related error handlers..." rg -l "token.*expired|403.*error" --type ts # Test 2: Verify consistent error handling across the codebase echo "Checking for consistency in error handling..." rg "doWithToken" -A 5 -B 5 --type ts # Test 3: Look for potential token-related tests echo "Checking for token-related tests..." fd -e spec.ts -e test.ts | xargs rg "token|403"Length of output: 54380
| return tokenPromise.then(fn).catch(async (error) => { | ||
| if (error.status === 403) { | ||
| console.warn("Token expired. Requesting new token..."); | ||
| tokenPromise = requestUserToken(); | ||
|
|
||
| // If three new tokens did not fix the 403, abort, otherwise we'll get into an endless loop here | ||
| if (tries < 3) { | ||
| return doWithToken(fn, tries + 1); | ||
| // If using the url sharing token failed, we try the user specific token instead. | ||
| const result = await doWithToken(fn, tries + 1, false); | ||
| // Upon successful retry with own token, discard the url token. | ||
| if (useURLTokenIfAvailable) { | ||
| shouldUseURLToken = false; | ||
| } | ||
| return result; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and retry mechanism.
The retry logic could be improved in several ways:
- The magic number '3' should be a named constant
- Consider implementing exponential backoff for retries
- Error logging could be more informative about the retry attempt number
+const MAX_TOKEN_RETRY_ATTEMPTS = 3;
+const RETRY_BACKOFF_MS = 1000;
+
return tokenPromise.then(fn).catch(async (error) => {
if (error.status === 403) {
- console.warn("Token expired. Requesting new token...");
+ console.warn(`Token expired (attempt ${tries}/${MAX_TOKEN_RETRY_ATTEMPTS}). Requesting new token...`);
tokenPromise = requestUserToken();
- // If three new tokens did not fix the 403, abort, otherwise we'll get into an endless loop here
- if (tries < 3) {
+ // Retry with exponential backoff until max attempts reached
+ if (tries < MAX_TOKEN_RETRY_ATTEMPTS) {
+ await new Promise(resolve => setTimeout(resolve, RETRY_BACKOFF_MS * Math.pow(2, tries - 1)));
// If using the url sharing token failed, we try the user specific token instead.
const result = await doWithToken(fn, tries + 1, false);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return tokenPromise.then(fn).catch(async (error) => { | |
| if (error.status === 403) { | |
| console.warn("Token expired. Requesting new token..."); | |
| tokenPromise = requestUserToken(); | |
| // If three new tokens did not fix the 403, abort, otherwise we'll get into an endless loop here | |
| if (tries < 3) { | |
| return doWithToken(fn, tries + 1); | |
| // If using the url sharing token failed, we try the user specific token instead. | |
| const result = await doWithToken(fn, tries + 1, false); | |
| // Upon successful retry with own token, discard the url token. | |
| if (useURLTokenIfAvailable) { | |
| shouldUseURLToken = false; | |
| } | |
| return result; | |
| const MAX_TOKEN_RETRY_ATTEMPTS = 3; | |
| const RETRY_BACKOFF_MS = 1000; | |
| return tokenPromise.then(fn).catch(async (error) => { | |
| if (error.status === 403) { | |
| console.warn(`Token expired (attempt ${tries}/${MAX_TOKEN_RETRY_ATTEMPTS}). Requesting new token...`); | |
| tokenPromise = requestUserToken(); | |
| // Retry with exponential backoff until max attempts reached | |
| if (tries < MAX_TOKEN_RETRY_ATTEMPTS) { | |
| await new Promise(resolve => setTimeout(resolve, RETRY_BACKOFF_MS * Math.pow(2, tries - 1))); | |
| // If using the url sharing token failed, we try the user specific token instead. | |
| const result = await doWithToken(fn, tries + 1, false); | |
| // Upon successful retry with own token, discard the url token. | |
| if (useURLTokenIfAvailable) { | |
| shouldUseURLToken = false; | |
| } | |
| return result; |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
frontend/javascripts/admin/api/token.ts (1)
39-50: Add JSDoc documentation for enhanced maintainability.The function signature has been updated with a new parameter, but lacks documentation explaining the token precedence logic.
+/** + * Executes a function with an authentication token, handling token retrieval and retries. + * @param fn The function to execute with the token + * @param tries Current retry attempt number + * @param useURLTokenIfAvailable If true, attempts to use URL token before falling back to user token + * @returns A promise resolving to the function's result + * @throws Will throw after MAX_TOKEN_RETRY_ATTEMPTS if all retries fail + */ export async function doWithToken<T>( fn: (token: string) => Promise<T>, tries: number = 1, useURLTokenIfAvailable: boolean = true, ): Promise<T>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- frontend/javascripts/admin/api/token.ts (2 hunks)
🔇 Additional comments (1)
frontend/javascripts/admin/api/token.ts (1)
39-68: Verify token removal behavior in the UI.The code implements the token fallback mechanism, but we should verify that the URL is properly updated when the token is discarded.
| const MAX_TOKEN_RETRY_ATTEMPTS = 3; | ||
|
|
||
| let tokenPromise: Promise<string>; | ||
|
|
||
| let tokenRequestPromise: Promise<string> | null; | ||
| let shouldUseURLToken: boolean = true; |
There was a problem hiding this comment.
Consider thread-safe state management for token source.
While the constant MAX_TOKEN_RETRY_ATTEMPTS is a good addition, the mutable shouldUseURLToken flag could lead to race conditions in concurrent requests. Consider using a more thread-safe approach:
-let shouldUseURLToken: boolean = true;
+class TokenSourceManager {
+ private static instance: TokenSourceManager;
+ private useURLToken: boolean = true;
+
+ private constructor() {}
+
+ static getInstance(): TokenSourceManager {
+ if (!TokenSourceManager.instance) {
+ TokenSourceManager.instance = new TokenSourceManager();
+ }
+ return TokenSourceManager.instance;
+ }
+
+ shouldUseURLToken(): boolean {
+ return this.useURLToken;
+ }
+
+ disableURLToken(): void {
+ this.useURLToken = false;
+ }
+}Committable suggestion was skipped due to low confidence.
| return tokenPromise.then(fn).catch(async (error) => { | ||
| if (error.status === 403) { | ||
| console.warn("Token expired. Requesting new token..."); | ||
| console.warn( | ||
| `Token expired (attempt ${tries}/${MAX_TOKEN_RETRY_ATTEMPTS}). Requesting new token...`, | ||
| ); | ||
| tokenPromise = requestUserToken(); | ||
|
|
||
| // If three new tokens did not fix the 403, abort, otherwise we'll get into an endless loop here | ||
| if (tries < 3) { | ||
| return doWithToken(fn, tries + 1); | ||
| if (tries < MAX_TOKEN_RETRY_ATTEMPTS) { | ||
| // If using the url sharing token failed, we try the user specific token instead. | ||
| const result = await doWithToken(fn, tries + 1, false); | ||
| // Upon successful retry with own token, discard the url token. | ||
| if (useURLTokenIfAvailable) { | ||
| shouldUseURLToken = false; | ||
| } | ||
| return result; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and logging for better debuggability.
While the retry logic is sound, the error handling and logging could be more comprehensive to aid debugging.
return tokenPromise.then(fn).catch(async (error) => {
if (error.status === 403) {
console.warn(
`Token expired (attempt ${tries}/${MAX_TOKEN_RETRY_ATTEMPTS}). Requesting new token...`,
);
tokenPromise = requestUserToken();
if (tries < MAX_TOKEN_RETRY_ATTEMPTS) {
const result = await doWithToken(fn, tries + 1, false);
if (useURLTokenIfAvailable) {
+ console.debug(
+ "Request succeeded with user token, disabling URL token for future requests"
+ );
shouldUseURLToken = false;
}
return result;
}
+ } else {
+ console.error(
+ "Request failed with non-403 error:",
+ error.status,
+ error.message
+ );
}
throw error;
});Also consider adding error context to help diagnose issues:
-throw error;
+const enhancedError = new Error(
+ `Request failed after ${tries} attempts: ${error.message}`
+);
+enhancedError.cause = error;
+throw enhancedError;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return tokenPromise.then(fn).catch(async (error) => { | |
| if (error.status === 403) { | |
| console.warn("Token expired. Requesting new token..."); | |
| console.warn( | |
| `Token expired (attempt ${tries}/${MAX_TOKEN_RETRY_ATTEMPTS}). Requesting new token...`, | |
| ); | |
| tokenPromise = requestUserToken(); | |
| // If three new tokens did not fix the 403, abort, otherwise we'll get into an endless loop here | |
| if (tries < 3) { | |
| return doWithToken(fn, tries + 1); | |
| if (tries < MAX_TOKEN_RETRY_ATTEMPTS) { | |
| // If using the url sharing token failed, we try the user specific token instead. | |
| const result = await doWithToken(fn, tries + 1, false); | |
| // Upon successful retry with own token, discard the url token. | |
| if (useURLTokenIfAvailable) { | |
| shouldUseURLToken = false; | |
| } | |
| return result; | |
| return tokenPromise.then(fn).catch(async (error) => { | |
| if (error.status === 403) { | |
| console.warn( | |
| `Token expired (attempt ${tries}/${MAX_TOKEN_RETRY_ATTEMPTS}). Requesting new token...`, | |
| ); | |
| tokenPromise = requestUserToken(); | |
| // If three new tokens did not fix the 403, abort, otherwise we'll get into an endless loop here | |
| if (tries < MAX_TOKEN_RETRY_ATTEMPTS) { | |
| // If using the url sharing token failed, we try the user specific token instead. | |
| const result = await doWithToken(fn, tries + 1, false); | |
| // Upon successful retry with own token, discard the url token. | |
| if (useURLTokenIfAvailable) { | |
| console.debug( | |
| "Request succeeded with user token, disabling URL token for future requests" | |
| ); | |
| shouldUseURLToken = false; | |
| } | |
| return result; | |
| } | |
| } else { | |
| console.error( | |
| "Request failed with non-403 error:", | |
| error.status, | |
| error.message | |
| ); | |
| } | |
| const enhancedError = new Error( | |
| `Request failed after ${tries} attempts: ${error.message}` | |
| ); | |
| enhancedError.cause = error; | |
| throw enhancedError; |
This PR improves the request retry functionality by trying using the token from the URL only upon the first try. If this results in a 403 (permissions not sufficient), the new code retries with the personal token of the user and if that succeeds, the URL token is removed and the previous error toast regarding a potentially expired token is hidden.
URL of deployed dev instance (used for testing):
Steps to test:
Issues:
(Please delete unneeded items, merge only when none are left open)
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements