Added request monitor to browser extension #2763
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's Guide by SourceryThis pull request implements a new request monitor for the browser extension, allowing users to inspect GraphQL requests through a dedicated DevTools panel and import them into the Altair app. The changes include establishing a communication channel between the devtools panel and the Altair application using a ping/pong mechanism, refactoring the tab handling logic, adding a suite of new UI components to display and interact with monitored requests, and updating dependencies and configuration files. Sequence diagram for Altair Import Request MessagingsequenceDiagram
participant U as User
participant RM as Request Monitor UI
participant OIA as openInAltair
participant CR as Chrome Runtime
participant WAS as WebExtensionsService
participant AA as Altair App
U->>RM: Click "Open in Altair"
RM->>OIA: Invoke openInAltair()
OIA->>CR: Call openAltairApp() [ensure Altair App tab]
OIA->>CR: sendMessage({ type: 'ping' })
CR-->>WAS: Deliver 'ping' message
WAS->>CR: sendMessage({ type: 'pong' })
Note right of OIA: Callback listener waits for 'pong' or 'ready'
CR-->>OIA: deliver 'pong' (or 'ready')
OIA->>CR: sendMessage({ type: 'import-window', window: data })
CR-->>WAS: Deliver 'import-window' message
WAS->>AA: Process import-window message
AA->>AA: importWindowData(message.window)
Note over AA: Request data imported into Altair app
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @imolorhe - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a visual indicator in the UI to show when the Altair app is ready to receive messages from the extension.
- The pnpm-lock.yaml file has a lot of changes; consider reviewing them to ensure they are all intentional.
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| await chrome.tabs.update(tabId, updateProperties); | ||
| }; | ||
|
|
||
| export const openAltairApp = async () => { |
There was a problem hiding this comment.
suggestion: Refactored tab management using helper methods.
The abstraction for creating, focusing, and storing tab IDs improves code clarity. Consider handling errors, for example if chrome.tabs.get fails or storage session is unavailable.
Suggested implementation:
const focusTab = async (tabId: number) => {
const updateProperties = { active: true };
try {
await chrome.tabs.update(tabId, updateProperties);
} catch (error) {
console.error(`Error focusing tab ${tabId}:`, error);
}
};export const getTabId = async () => {
try {
const data = await chrome.storage.session.get([ALTAIR_APP_TAB_ID_STORAGE_KEY]);
return data[ALTAIR_APP_TAB_ID_STORAGE_KEY] as number | undefined;
} catch (error) {
console.error("Error retrieving tab ID from storage:", error);
return undefined;
}
};const setTabId = async (tabId: number) => {
try {
await chrome.storage.session.set({ [ALTAIR_APP_TAB_ID_STORAGE_KEY]: tabId });
} catch (error) {
console.error(`Error setting tab ID ${tabId} in storage:`, error);
}
};export const removeTabId = async () => {
try {
await chrome.storage.session.remove(ALTAIR_APP_TAB_ID_STORAGE_KEY);
} catch (error) {
console.error("Error removing tab ID from storage:", error);
}
};| }; | ||
| browser.runtime.onMessage.addListener(callback); | ||
|
|
||
| setTimeout(() => { |
There was a problem hiding this comment.
question: Listener cleanup logic in openInAltair using setTimeout.
The removal of the message listener after 10 seconds is a pragmatic way to avoid stale listeners. Consider whether this timeout sufficiently covers slower initialization scenarios.
| setRequests((prevRequests) => [...prevRequests, data]); | ||
| }); | ||
|
|
||
| // TODO: Add cleanup logic |
There was a problem hiding this comment.
issue (bug_risk): Event listener cleanup needs addressing.
The hook registers a listener on chrome.devtools.network.onRequestFinished but does not currently unregister it on component unmount. Implementing cleanup would prevent potential memory leaks.
| const ast = parse(query); | ||
| return ast.definitions | ||
| .filter((def): def is OperationDefinitionNode => | ||
| def.kind === Kind.OPERATION_DEFINITION && def.name ? true : false |
There was a problem hiding this comment.
suggestion (code-quality): Avoid unneeded ternary statements (simplify-ternary)
Suggested change
| def.kind === Kind.OPERATION_DEFINITION && def.name ? true : false | |
| !!(def.kind === Kind.OPERATION_DEFINITION && def.name) |
Explanation
It is possible to simplify certain ternary statements into either use of an|| or !.This makes the code easier to read, since there is no conditional logic.
|
Visit the preview URL for this PR (updated for commit 6875782): https://altair-gql--pr2763-imolorhe-add-crx-mon-3r66b5cv.web.app (expires Wed, 19 Feb 2025 20:52:59 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 02d6323d75a99e532a38922862e269d63351a6cf |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Added a devtools panel to monitor for GraphQL requests and allows opening the request in Altair.
Setup a web extension service, which sets up a listener for messages from the devtools panel. To import request data from the devtools panel, we open the Altair app, then send a message to the Altair app which is part of the extension (so can be considered an extension page) using the
chrome.runtime.sendMessagemethod. The extension page processes the message and imports the request data into the Altair app.To make sure the Altair app is loaded and ready to receive the message, we send a ping message to the Altair app, and wait for the pong message before sending the actual message. We also wait for the ready message from the Altair app in case it is not loaded yet to have received the ping message.
Checks
yarn test-buildChanges proposed in this pull request:
Summary by Sourcery
Add a GraphQL request monitor to the browser extension.
New Features:
Tests: