fix auth edge case #180
fix auth edge case #180
Conversation
WalkthroughWalkthroughThe changes in this pull request enhance the authentication handling and message communication within the Chrome extension's background script. New functions for authentication management, including retrieving and clearing authentication data, have been added. Additionally, modifications to the message handling logic improve responses when a tab ID is unavailable, ensuring better management of authentication states. Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Poem
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Files not reviewed due to no reviewable changes (1)
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: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- apps/chrome-extension/src/pages/background/index.ts (1 hunks)
- apps/mocksi-lite-next/src/pages/background/index.ts (1 hunks)
Additional context used
Biome
apps/chrome-extension/src/pages/background/index.ts
[error] 53-74: Promise executor functions should not be
async.(lint/suspicious/noAsyncPromiseExecutor)
Additional comments not posted (6)
apps/mocksi-lite-next/src/pages/background/index.ts (1)
140-148: LGTM!The new conditional block correctly handles the scenario where
tab.idis undefined. It ensures that authentication is properly initiated by callingshowAuthTab(true)and sends an appropriate response indicating that authentication is in progress.This change improves the robustness of the authentication process and provides a clear response when a tab is not available.
apps/chrome-extension/src/pages/background/index.ts (5)
11-34: LGTM!The function correctly handles the case where the access token is expired by clearing the authentication data. It also uses a try-catch block to handle errors and returns null if there is no authentication data or if an error occurs.
36-43: LGTM!The function correctly clears the authentication data from Chrome storage and uses a try-catch block to handle errors.
45-50: LGTM!The function correctly retrieves the current active tab using the
chrome.tabs.querymethod and returns it.
85-106: LGTM!The function correctly handles the user clicking the extension icon by sending a message to the current tab to mount the extension, sending a message with the previous request data and message if there is a previous request, and setting the extension icon to "play-icon.png" if the previous request message is "PLAY".
119-203: LGTM!The function correctly handles the different types of messages received from an external source:
- For "AUTH_ERROR" messages, it clears the authentication data and sends a response with the message "retry".
- For "UNAUTHORIZED" messages, it retrieves the authentication data and sends a response with the access token, email, and current tab URL if the authentication data exists, or opens the authentication tab and sends a response with the message "authenticating" if the authentication data doesn't exist.
- For other messages, it sends a message to the current tab with the request data and message and sets the extension icon based on the message.
It also correctly stores the last app state in the
prevRequestvariable if the message is not "MINIMIZED".
| // FIXME: there's duplicated code belows. | ||
| if (!tab.id) { |
There was a problem hiding this comment.
Offer assistance to fix the duplicated code.
There is a TODO comment indicating that there is duplicated code that needs to be fixed. I can help you refactor the code to remove the duplication.
Do you want me to suggest a refactoring or open a GitHub issue to track this task?
| async function showAuthTab(force?: boolean) { | ||
| return new Promise(async (resolve: (value?: unknown) => void) => { | ||
| chrome.tabs.query({}, function (tabs) { | ||
| let tabExists = false; | ||
| if (!force) { | ||
| for (const tab of tabs) { | ||
| const tabUrlStr = tab.url || tab.pendingUrl || ""; | ||
| const loadUrl = new URL(import.meta.env.VITE_NEST_APP); | ||
| const tabUrl = new URL(tabUrlStr); | ||
| if (loadUrl.href === tabUrl.href) { | ||
| tabExists = true; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (!tabExists) { | ||
| chrome.tabs.create({ url: import.meta.env.VITE_NEST_APP }, resolve); | ||
| } else { | ||
| resolve(); | ||
| } | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Fix the async promise executor function.
The static analysis tool has flagged that the promise executor function should not be async. This is because the async function will return a promise, which is not the expected return type for a promise executor function.
To fix this, you can remove the async keyword from the promise executor function and use .then() to handle the asynchronous code instead:
- return new Promise(async (resolve: (value?: unknown) => void) => {
+ return new Promise((resolve: (value?: unknown) => void) => {
chrome.tabs.query({}, function (tabs) {
// ...
- if (!tabExists) {
- chrome.tabs.create({ url: import.meta.env.VITE_NEST_APP }, resolve);
- } else {
- resolve();
- }
+ chrome.tabs.create({ url: import.meta.env.VITE_NEST_APP }).then(resolve);
});
});Committable suggestion was skipped due to low confidence.
Tools
Biome
[error] 53-74: Promise executor functions should not be
async.(lint/suspicious/noAsyncPromiseExecutor)
elg0nz
force-pushed
the
fix-auth-edge-case
branch
from
14dc39f to
49e52f6
Compare
Summary by CodeRabbit
New Features
Bug Fixes
Chores