feat(publishing): frendly error message for 401 #257
Conversation
Signed-off-by: wheat <[email protected]>
Summary of ChangesHello @wheatliu, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the error handling mechanism for OAuth2 authentication across several social media platform integrations. By introducing a dedicated Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new PlatformAuthExpiredException to standardize handling of 401 authentication errors, which is a great step towards more consistent error management. The changes replace various platform-specific error handling mechanisms with this new exception. My review focuses on enhancing this new exception to include contextual information, such as the accountId, which was available in the previous logging statements. This will improve debuggability and make the new error handling more robust.
| constructor(platform: string) { | ||
| super(platform, 'GetAccessToken', 'AuthError', 'OAuth2 credential expired, please re-authorize', 401, 401, undefined, false, undefined) | ||
| } |
There was a problem hiding this comment.
To improve debuggability, consider adding a context parameter to the constructor. This allows passing valuable information like accountId when the exception is thrown, which is useful for logging and error tracking. The previous implementation in some services logged this information, and this change would preserve that context.
| constructor(platform: string) { | |
| super(platform, 'GetAccessToken', 'AuthError', 'OAuth2 credential expired, please re-authorize', 401, 401, undefined, false, undefined) | |
| } | |
| constructor(platform: string, context?: { accountId: string }) { | |
| super(platform, 'GetAccessToken', 'AuthError', 'OAuth2 credential expired, please re-authorize', 401, 401, undefined, false, context) | |
| } |
| if (!credential.access_token) { | ||
| return Promise.resolve('') | ||
| if (!credential || !credential.access_token) { | ||
| throw new PlatformAuthExpiredException(this.platform) |
There was a problem hiding this comment.
For better debugging, please pass the accountId in the exception's context. This will require the suggested change to PlatformAuthExpiredException to accept a context object.
throw new PlatformAuthExpiredException(this.platform, { accountId })| }) | ||
| if (!oauth2Credential) { | ||
| return null | ||
| throw new PlatformAuthExpiredException(this.platform) |
There was a problem hiding this comment.
For better debugging, please pass the accountId in the exception's context. This will require the suggested change to PlatformAuthExpiredException to accept a context object.
throw new PlatformAuthExpiredException(this.platform, { accountId })| if (isRefreshTokenExpired) { | ||
| this.logger.warn(`Kwai account ${accountId} refresh_token is expired, expired at: ${accessTokenInfo.refresh_token_expires_in}`) | ||
| return null | ||
| throw new PlatformAuthExpiredException(this.platform) |
There was a problem hiding this comment.
For better debugging, please pass the accountId in the exception's context. This will require the suggested change to PlatformAuthExpiredException to accept a context object.
throw new PlatformAuthExpiredException(this.platform, { accountId })| if (!newAccountToken) { | ||
| this.logger.warn(`Kwai account ${accountId} access_token refresh failed`) | ||
| return null | ||
| throw new PlatformAuthExpiredException(this.platform) |
There was a problem hiding this comment.
For better debugging, please pass the accountId in the exception's context. This will require the suggested change to PlatformAuthExpiredException to accept a context object.
throw new PlatformAuthExpiredException(this.platform, { accountId })| if (!credential || !credential.access_token) { | ||
| this.logger.error(`youtube credential not found for accountId: ${accountId}`) | ||
| throw new AppException(ResponseCode.ChannelCredentialNotFound, { channel: 'youtube', accountId }) | ||
| throw new PlatformAuthExpiredException(this.platform) |
There was a problem hiding this comment.
For better debugging, please pass the accountId in the exception's context. This will require the suggested change to PlatformAuthExpiredException to accept a context object.
throw new PlatformAuthExpiredException(this.platform, { accountId })| if (!credential.refresh_token) { | ||
| this.logger.error(`refresh Token not found for accountId: ${accountId}`) | ||
| throw new AppException(ResponseCode.ChannelRefreshTokenNotFound, { channel: 'youtube', accountId }) | ||
| throw new PlatformAuthExpiredException(this.platform) |
There was a problem hiding this comment.
For better debugging, please pass the accountId in the exception's context. This will require the suggested change to PlatformAuthExpiredException to accept a context object.
throw new PlatformAuthExpiredException(this.platform, { accountId })| const errMsg = `youtube refresh Token expired for accountId: ${accountId}, expired at: ${credential.refresh_expires_in}, please re-authorize` | ||
| this.logger.error(errMsg) | ||
| throw new AppException(ResponseCode.ChannelRefreshTokenExpired, { channel: 'youtube', accountId, credential }) | ||
| throw new PlatformAuthExpiredException(this.platform) |
There was a problem hiding this comment.
For better debugging, please pass the accountId in the exception's context. This will require the suggested change to PlatformAuthExpiredException to accept a context object.
throw new PlatformAuthExpiredException(this.platform, { accountId })| if (!accessToken) { | ||
| this.logger.error(`refresh Token failed for accountId: ${accountId}`) | ||
| throw new AppException(ResponseCode.ChannelRefreshTokenFailed, { accountId }) | ||
| throw new PlatformAuthExpiredException(this.platform) |
There was a problem hiding this comment.
For better debugging, please pass the accountId in the exception's context. This will require the suggested change to PlatformAuthExpiredException to accept a context object.
throw new PlatformAuthExpiredException(this.platform, { accountId })|
|
||
| if (!accessToken) { | ||
| return false | ||
| throw new PlatformAuthExpiredException(this.platform) |
There was a problem hiding this comment.
For better debugging, please pass the accountId in the exception's context. This will require the suggested change to PlatformAuthExpiredException to accept a context object.
throw new PlatformAuthExpiredException(this.platform, { accountId })
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
There was a problem hiding this comment.
Pull Request Overview
This PR standardizes authentication error handling across all platform services by introducing a unified PlatformAuthExpiredException that returns a 401 status code with a user-friendly message "OAuth2 credential expired, please re-authorize". The changes replace various inconsistent error patterns (returning null, returning false, throwing different exceptions) with this single exception type.
- Introduces
PlatformAuthExpiredExceptionclass for consistent auth error handling - Replaces platform-specific error codes and messages with a unified 401 response
- Removes redundant null checks that are now handled by throwing exceptions
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| platform.exception.ts | New exception class for OAuth2 authentication failures with 401 status code |
| youtube.service.ts | Replaces multiple auth error codes with PlatformAuthExpiredException; changes ensureValidAccessToken to throw instead of returning false |
| twitter.service.ts | Removes null checks and error handling in authorize and getOAuth2Credential methods |
| tiktok.service.ts | Removes unused imports and simplifies token refresh error handling |
| pinterest.service.ts | Replaces null returns with exception throws in authorize method |
| threads.service.ts | Removes null checks and error logging in favor of exception throwing |
| linkedin.service.ts | Standardizes error handling for token refresh failures |
| instagram.service.ts | Replaces generic Error throws with PlatformAuthExpiredException |
| facebook.service.ts | Standardizes error handling for token refresh and save failures |
| meta/base.service.ts | Replaces null return with exception throw in getOAuth2Credential |
| kwai.service.ts | Removes null checks and standardizes auth error handling |
| bilibili.service.ts | Changes empty string returns to exception throws for missing credentials |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const newAccountToken = await this.kwaiApiService.refreshToken( | ||
| accessTokenInfo.refresh_token, | ||
| ) | ||
| if (!newAccountToken) { |
There was a problem hiding this comment.
[nitpick] The removed warning log provided context about refresh failure. Consider adding logging before throwing the exception:
this.logger.warn(`Kwai account ${accountId} access_token refresh failed`)
throw new PlatformAuthExpiredException(this.platform)| if (!newAccountToken) { | |
| if (!newAccountToken) { | |
| this.logger.warn(`Kwai account ${accountId} access_token refresh failed`) |
|
|
||
| if (!accessToken) { | ||
| return false | ||
| throw new PlatformAuthExpiredException(this.platform) |
There was a problem hiding this comment.
The behavior of ensureValidAccessToken has changed from returning false to throwing PlatformAuthExpiredException. This change creates unreachable code at line 2077-2078. Since getUserAccessToken (called at line 2075) will now throw PlatformAuthExpiredException when there's no access token, the null check at line 2077 will never be false. The function will either return true at line 2081 or throw an exception.
Consider either:
- Removing the unreachable null check (lines 2077-2078)
- Updating all callers that expect boolean returns (there are 32 call sites like
if (!(await this.ensureValidAccessToken(accountId)))) to handle exceptions instead
The current implementation makes the function's return type misleading - it's declared as Promise<boolean> but will never return false.
| } | ||
|
|
||
| const isRefreshTokenExpired = credential.refresh_expires_in && credential.refresh_expires_in <= getCurrentTimestamp() | ||
| if (isRefreshTokenExpired) { |
There was a problem hiding this comment.
[nitpick] The removed error logging included useful debugging information (expiration timestamp). Consider adding similar logging before throwing the exception to help with troubleshooting:
this.logger.error(`youtube refresh Token expired for accountId: ${accountId}, expired at: ${credential.refresh_expires_in}`)
throw new PlatformAuthExpiredException(this.platform)| if (isRefreshTokenExpired) { | |
| if (isRefreshTokenExpired) { | |
| this.logger.error(`youtube refresh Token expired for accountId: ${accountId}, expired at: ${credential.refresh_expires_in}`) |
| // 判断 refresh_token 是否过期 | ||
| const isRefreshTokenExpired | ||
| = getCurrentTimestamp() >= accessTokenInfo.refresh_token_expires_in | ||
| if (isRefreshTokenExpired) { |
There was a problem hiding this comment.
[nitpick] The removed warning log included helpful debugging context (expiration timestamp). Consider adding logging before throwing the exception:
this.logger.warn(`Kwai account ${accountId} refresh_token is expired, expired at: ${accessTokenInfo.refresh_token_expires_in}`)
throw new PlatformAuthExpiredException(this.platform)| if (isRefreshTokenExpired) { | |
| if (isRefreshTokenExpired) { | |
| this.logger.warn(`Kwai account ${accountId} refresh_token is expired, expired at: ${accessTokenInfo.refresh_token_expires_in}`) |
Signed-off-by: wheat <[email protected]>
Pull Request
🧩 Issue Link
Closes #237
📝 Description
frendly error message for 401