Tabs feature #27
Tabs feature #27
Conversation
…s to gitignore for development.
…or this (tabs, history, activeTab). Added chrome.png. Added the connection to connection_manager. Added background script in background.ts to get all tabs in the browser. This connection also has 2 error handlers (not enough tabs and no tabs found) that more-readably show to the user when either is the case.
…b contents for better space creation. Added scripting permission to package.json to allow for tab reading to occur.
Summary of ChangesHello @Vodiboi, 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 introduces a major new capability to integrate and analyze a user's open Chrome browser tabs. It enables the application to gather information from active web pages, process their content, and present it in a structured 'space'. The changes encompass significant additions to the background script for tab and content handling, updates to extension permissions, and a new UI component for user interaction, alongside general improvements to data processing and error resilience. 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 'Tabs' feature, which creates a Mantis space based on the content of currently open browser tabs. The implementation includes a new background script logic to fetch tab content, a new connection ChromeTabsConnection, and UI integration on the Google search page. Additionally, it refactors UUID generation by removing the uuid package in favor of a centralized helper.
My review focuses on a few key areas. There's a critical issue in src/background.ts where multiple onMessage listeners are used, which should be consolidated into one to prevent unpredictable behavior. I've also pointed out some opportunities to improve code quality in the new chromeTabs connection by using specific types instead of any and adhering to best practices for event handling. Using console.error for logging errors is also recommended for better diagnostics.
| chrome.runtime.onMessage.addListener((request, sender, sendResponse) => { | ||
| // Handle getTabs request for Chrome Tabs connection | ||
| if (request.action === "getTabs") { | ||
| chrome.tabs.query({}, (tabs) => { | ||
| if (chrome.runtime.lastError) { | ||
| sendResponse({ error: chrome.runtime.lastError.message }); | ||
| } else { | ||
| sendResponse({ tabs: tabs }); | ||
| } | ||
| }); | ||
| return true; | ||
| } | ||
|
|
||
| // Handle getTabsWithContent request | ||
| if (request.action === "getTabsWithContent") { | ||
| chrome.tabs.query({}, async (tabs) => { | ||
| if (chrome.runtime.lastError) { | ||
| sendResponse({ error: chrome.runtime.lastError.message }); | ||
| return; | ||
| } | ||
|
|
||
| const tabsWithContent = []; | ||
|
|
||
| for (const tab of tabs) { | ||
| const tabData = { ...tab, pageContent: '' }; // Add pageContent property | ||
|
|
||
| // Try to get page content for each tab | ||
| try { | ||
| if (tab.id && tab.url && !tab.url.startsWith('chrome://') && !tab.url.startsWith('chrome-extension://')) { | ||
| // Execute content script to get page text | ||
| const results = await chrome.scripting.executeScript({ | ||
| target: { tabId: tab.id }, | ||
| func: getPageContent, // Use 'func' instead of 'function' | ||
| }); | ||
|
|
||
| if (results && results[0] && results[0].result) { | ||
| tabData.pageContent = results[0].result; | ||
| } | ||
| } | ||
| } catch (error) { | ||
| console.log(`Could not get content for tab ${tab.id}:`, error); | ||
| // Set a fallback description | ||
| tabData.pageContent = `Content from ${tab.url ? new URL(tab.url).hostname : 'unknown site'} - unable to read page content`; | ||
| } | ||
|
|
||
| tabsWithContent.push(tabData); | ||
| } | ||
|
|
||
| sendResponse({ tabs: tabsWithContent }); | ||
| }); | ||
| return true; | ||
| } | ||
|
|
||
| // Don't interfere with other message handlers | ||
| return false; | ||
| }); |
There was a problem hiding this comment.
You've added a new chrome.runtime.onMessage.addListener. However, this file now contains multiple onMessage listeners (e.g., another one starts on line 169, and more follow). Registering multiple listeners for the onMessage event is an anti-pattern in Chrome extension development and can lead to unpredictable behavior, race conditions, or messages not being handled as expected. Only one listener should be active for the runtime.onMessage event.
It's highly recommended to consolidate all message handling logic into a single chrome.runtime.onMessage.addListener block, using if/else if or a switch statement to delegate based on request.action.
Example of consolidation:
chrome.runtime.onMessage.addListener((request, sender, sendResponse) => {
switch (request.action) {
case "getTabs":
// ... logic for getTabs
return true; // Indicate async response
case "getTabsWithContent":
// ... logic for getTabsWithContent
return true; // Indicate async response
case "setCookie":
// ... logic for setCookie
return true; // Indicate async response
// ... other cases
default:
// Optional: handle unknown actions
break;
}
return false;
});There was a problem hiding this comment.
Is this something that should be fixed Luca? This problem predates my tabs feature, so I assume you did this intentionally...
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
type fix Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
Hello @Vodiboi! I'm here to help. Do you have any questions about the summary I provided, or anything else regarding this pull request? |
There was a problem hiding this comment.
Code Review
This pull request introduces a new 'Tabs' feature that creates a Mantis space from the content of all open browser tabs. The implementation includes adding a new Chrome Tabs connection, background script logic to fetch tab content, and the necessary permissions. The pull request also includes a good refactoring to replace the uuid package with the native crypto.randomUUID().
My review focuses on a critical issue in the new background script message listener that could break existing functionality. I've also provided suggestions to improve error messaging for a better user experience and to enhance type safety in the new connection for improved code quality and maintainability.
There was a problem hiding this comment.
Code Review
This pull request introduces a new 'Tabs' feature, allowing users to create a 'space' from their open browser tabs. The implementation includes fetching tab data, extracting content via a content script, and adding a new connection. The PR also refactors UUID generation, which is a good improvement.
My review focuses on several key areas:
- Architecture: I've pointed out an issue with using multiple
onMessagelisteners inbackground.ts, which should be consolidated for robustness. - Maintainability & Best Practices: In the new
chromeTabsconnection, I've suggested improvements regarding magic numbers, brittle CSS selectors, and string-based error handling. - Security & Code Quality: I've highlighted the use of
innerHTMLwith inline event handlers, which is insecure, and suggested safer alternatives. I also noted areas where code can be simplified and type safety can be improved.
Overall, the feature is well-implemented, and these suggestions aim to improve the long-term quality and stability of the code.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new "Tabs" feature, which creates a space based on the content of currently open browser tabs. It adds a new ChromeTabsConnection, a background script to fetch tab data, and updates permissions and dependencies accordingly. The removal of the uuid package in favor of the native crypto.randomUUID() is a good improvement. My review focuses on improving the robustness and maintainability of the new code, with suggestions to refactor message listeners in the background script, parallelize data fetching for better performance, and reduce code duplication in the new connection file.
| chrome.runtime.onMessage.addListener((request, sender, sendResponse) => { | ||
| // Handle getTabs request for Chrome Tabs connection | ||
| if (request.action === "getTabs") { | ||
| chrome.tabs.query({}, (tabs) => { | ||
| if (chrome.runtime.lastError) { | ||
| sendResponse({ error: chrome.runtime.lastError.message }); | ||
| } else { | ||
| sendResponse({ tabs: tabs }); | ||
| } | ||
| }); | ||
| return true; | ||
| } | ||
|
|
||
| // Handle getTabsWithContent request | ||
| if (request.action === "getTabsWithContent") { | ||
| chrome.tabs.query({}, async (tabs) => { | ||
| if (chrome.runtime.lastError) { | ||
| sendResponse({ error: chrome.runtime.lastError.message }); | ||
| return; | ||
| } | ||
|
|
||
| const tabsWithContent = []; | ||
|
|
||
| for (const tab of tabs) { | ||
| const tabData = { ...tab, pageContent: '' }; // Add pageContent property | ||
|
|
||
| // Try to get page content for each tab | ||
| try { | ||
| if (tab.id && tab.url && !tab.url.startsWith('chrome://') && !tab.url.startsWith('chrome-extension://')) { | ||
| // Execute content script to get page text | ||
| const results = await chrome.scripting.executeScript({ | ||
| target: { tabId: tab.id }, | ||
| func: getPageContent, // Use 'func' instead of 'function' | ||
| }); | ||
|
|
||
| if (results && results[0] && results[0].result) { | ||
| tabData.pageContent = results[0].result; | ||
| } | ||
| } | ||
| } catch (error) { | ||
| console.error(`Could not get content for tab ${tab.id}:`, error); | ||
| // Set a fallback description | ||
| tabData.pageContent = `Content from ${tab.url ? new URL(tab.url).hostname : 'unknown site'} - unable to read page content`; | ||
| } | ||
|
|
||
| tabsWithContent.push(tabData); | ||
| } | ||
|
|
||
| sendResponse({ tabs: tabsWithContent }); | ||
| }); | ||
| return true; | ||
| } | ||
|
|
||
| // Don't interfere with other message handlers | ||
| return false; | ||
| }); |
There was a problem hiding this comment.
While adding a new message listener for the tabs feature works, having multiple chrome.runtime.onMessage.addListener calls in the same background script can make it harder to manage and debug. Some of the existing listeners also don't explicitly return a value if the request.action doesn't match, which can lead to the message port closing prematurely.
It would be more robust and maintainable to consolidate all message handling into a single listener that uses a switch statement to delegate actions. This ensures clear logic flow and correct handling of asynchronous responses.
There was a problem hiding this comment.
Luca this issue predates my contributions, I assume you did it intentionally, and changing it would probably involve changing stuff for all of the different integrations, not just the tabs one... Should I?
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new 'Tabs' feature, allowing users to create a Mantis space from their currently open browser tabs. The implementation includes a new background script logic to fetch tab content, a new connection component (ChromeTabsConnection) with UI injection on Google search pages, and robust error handling with user-facing notifications. The uuid dependency has also been beneficially removed in favor of the native crypto.randomUUID().
My review focuses on improving the robustness and maintainability of the new code. The most critical issue is the use of multiple chrome.runtime.onMessage.addListener instances in the background script, which can cause runtime errors and should be consolidated. Other suggestions include making DOM selectors less brittle, improving the retry strategy, and refactoring complex functions for better clarity.
| chrome.runtime.onMessage.addListener((request, sender, sendResponse) => { | ||
| // Handle getTabs request for Chrome Tabs connection | ||
| if (request.action === "getTabs") { | ||
| chrome.tabs.query({}, (tabs) => { | ||
| if (chrome.runtime.lastError) { | ||
| sendResponse({ error: chrome.runtime.lastError.message }); | ||
| } else { | ||
| sendResponse({ tabs: tabs }); | ||
| } | ||
| }); | ||
| return true; | ||
| } | ||
|
|
||
| // Handle getTabsWithContent request | ||
| if (request.action === "getTabsWithContent") { | ||
| chrome.tabs.query({}, async (tabs) => { | ||
| if (chrome.runtime.lastError) { | ||
| sendResponse({ error: chrome.runtime.lastError.message }); | ||
| return; | ||
| } | ||
|
|
||
| const tabsWithContentPromises = tabs.map(async (tab) => { | ||
| const tabData = { ...tab, pageContent: '' }; // Add pageContent property | ||
|
|
||
| // Try to get page content for each tab | ||
| try { | ||
| if (tab.id && tab.url && !tab.url.startsWith('chrome://') && !tab.url.startsWith('chrome-extension://')) { | ||
| // Execute content script to get page text | ||
| const results = await chrome.scripting.executeScript({ | ||
| target: { tabId: tab.id }, | ||
| func: getPageContent, // Use 'func' instead of 'function' | ||
| }); | ||
|
|
||
| if (results && results[0] && results[0].result) { | ||
| tabData.pageContent = results[0].result; | ||
| } | ||
| } | ||
| } catch (error) { | ||
| console.error(`Could not get content for tab ${tab.id}:`, error); | ||
| // Set a fallback description | ||
| tabData.pageContent = `Content from ${tab.url ? new URL(tab.url).hostname : 'unknown site'} - unable to read page content`; | ||
| } | ||
|
|
||
| return tabData; | ||
| }); | ||
|
|
||
| const tabsWithContent = await Promise.all(tabsWithContentPromises); | ||
|
|
||
| sendResponse({ tabs: tabsWithContent }); | ||
| }); | ||
| return true; | ||
| } | ||
|
|
||
| // Don't interfere with other message handlers | ||
| return false; | ||
| }); |
There was a problem hiding this comment.
Adding multiple chrome.runtime.onMessage.addListener calls in the same context (like a background script) is an anti-pattern that can lead to race conditions and unexpected behavior. When a message is sent, every registered listener will be triggered. If more than one listener attempts to call sendResponse for the same message, Chrome will raise an error. All message handling logic for chrome.runtime.onMessage should be consolidated into a single listener. This listener can then use a switch statement or an if/else if chain based on request.action to delegate to the appropriate handler function. This file now contains several listeners, which should be refactored into one.
There was a problem hiding this comment.
Copy & Pasting my responses to this from earlier:
First code review: Is this something that should be fixed Luca? This problem predates my tabs feature, so I assume you did this intentionally...
Second code review: Luca this issue predates my contributions, I assume you did it intentionally, and changing it would probably involve changing stuff for all of the different integrations, not just the tabs one... Should I?
1 participant
Now looks at the first 300 chars of text from a page and makes a space using it. Removed domain stuff and duplicating entries.