Add content script palette#3
Conversation
There was a problem hiding this comment.
Hello @ackkerman, 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!
Summary of Changes
Hello! Gemini here, providing a summary of this pull request. The main goal of this PR, as indicated by the title and description, is to introduce a command palette feature accessible via a content script injected into web pages. This palette allows users to quickly search and access bookmarks. The palette is designed to appear as a centered modal. The PR also includes changes to allow opening this same palette within the extension's popup and a minor style adjustment for the popup's width.
Highlights
- Content Script Injection: A new content script (
src/contentScript/index.tsx) is added and configured invite.config.tsto be injected into all web pages (<all_urls>). - Keyboard Shortcut Activation: The injected content script listens for the keyboard shortcut
Ctrl+Alt+Kto open the command palette modal andEscapeto close it. - Reusable Command Palette Component: A new
CommandPaletteReact component is created (src/popup/components/CommandPalette.tsx) using Mantine'sModal. This component integrates existing bookmark search functionality. - Popup Integration: The
CommandPalettecomponent is also integrated into the main popup application (src/popup/App.tsx) and can be opened there using theAlt+kkeyboard shortcut. - Popup Styling Adjustment: The CSS selector for setting the popup's width is updated from targeting the
bodyelement to targeting the#rootelement with a specific class (#root.extension-popup).
Changelog
Click here to see the changelog
- public/index.html
- Added the
extension-popupclass to the#rootdiv (line 11).
- Added the
- src/contentScript/index.tsx
- New file created.
- Sets up a new DOM element (
moskeyto-command-palette-container) and appends it to the document body (lines 7-14). - Renders a React application (
App) into this container usingReactDOM.createRoot(line 15, 40). - The
Appcomponent listens forCtrl+Alt+Kto open theCommandPaletteandEscapeto close it (lines 20-31). - Renders the
CommandPalettecomponent within aMantineProvider(lines 34-36).
- src/popup/App.tsx
- Imports the
CommandPalettecomponent (line 15). - Adds state (
paletteOpened) to manage the visibility of the command palette (line 23). - Adds a keyboard event listener for
Alt+kto open the palette andEscapeto close it within the popup context (lines 46-51). - Renders the
CommandPalettecomponent, controlled by thepaletteOpenedstate (line 78).
- Imports the
- src/popup/components/CommandPalette.tsx
- New file created.
- Defines the
CommandPalettefunctional component (line 13). - Uses Mantine's
Modalcomponent for the palette UI (line 28). - Integrates bookmark search functionality using
useKeywordDataanduseBookmarkDatahooks (lines 14-15). - Renders a
BookmarkListFormfor input and aListofBookmarkListItemcomponents (lines 29-40). - Includes a
useEffecthook to automatically focus the input field when the modal is opened (lines 19-25).
- src/popup/index.css
- Changed the CSS selector for the popup width from
bodyto#root.extension-popup(line 27).
- Changed the CSS selector for the popup width from
- vite.config.ts
- Added a
content_scriptsentry to thedefineManifestconfiguration (lines 26-29). - Configured
src/contentScript/index.tsxto be injected into<all_urls>.
- Added a
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and 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 to provide feedback.
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.
Keys pressed just so,
A palette starts to glow,
Bookmarks appear fast.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature: a command palette accessible both within the extension popup and directly on web pages. The implementation of the core functionality, including keyboard shortcuts and modal display, is well done.
My main concerns, detailed below, relate to ensuring the content script's UI (the command palette on web pages) is properly isolated to prevent style conflicts with host pages. Addressing these will significantly improve the robustness and reliability of the extension across different websites.
Summary of Findings
- Critical: CSS Leakage from Content Script: The content script imports global CSS from
src/popup/index.css(seesrc/contentScript/index.tsx:5), which can lead to style conflicts on host pages by altering their appearance. This needs to be addressed by scoping CSS specifically for the command palette. - High: UI Isolation with Shadow DOM: For robust isolation of the command palette's UI when injected into web pages, using a Shadow DOM is highly recommended (see
src/contentScript/index.tsx:9-14). This prevents CSS and DOM interference between the content script and the host page. - Medium: Type Safety for
linkRefs: In the newCommandPalette.tsxcomponent (line 17), thelinkRefsuses theanytype. Specifying a more precise type, such asuseRef<(HTMLAnchorElement | null)[]>([]), would improve code quality and maintainability. - Low: Magic Number for z-index (Not commented due to severity settings): The
zIndexvalue2147483647insrc/contentScript/index.tsx(line 12) is a magic number. For better readability and maintainability, consider defining this as a named constant (e.g.,MAX_Z_INDEX). This was not added as a direct comment due to the configured review severity settings.
Merge Readiness
This pull request adds significant new functionality and is a great step forward. However, due to the critical issue of potential CSS conflicts caused by the content script's global style injection, and the high importance of UI isolation using Shadow DOM, I recommend that these changes be addressed before merging. The medium severity type issue should also be considered. I am unable to approve the pull request myself; please ensure these points are resolved and further review is conducted if necessary.
| import ReactDOM from 'react-dom/client'; | ||
| import { MantineProvider } from '@mantine/core'; | ||
| import { CommandPalette } from '../popup/components/CommandPalette'; | ||
| import '../popup/index.css'; |
There was a problem hiding this comment.
Importing ../popup/index.css here seems to introduce global styles from the popup (like body::-webkit-scrollbar styles and potentially others defined in src/popup/index.css) directly into host web pages. This could unintentionally alter the appearance or layout of websites where the extension is active, leading to a poor user experience or broken sites.
Could we explore ways to ensure styles for the command palette are strictly scoped and do not leak out or conflict with host page styles? For instance, creating a dedicated, scoped CSS file for the command palette component (perhaps using CSS Modules or a similar scoping mechanism) would be a safer approach.
| if (!container) { | ||
| container = document.createElement('div'); | ||
| container.id = containerId; | ||
| container.style.zIndex = '2147483647'; | ||
| document.body.appendChild(container); | ||
| } |
There was a problem hiding this comment.
To achieve robust UI isolation for the command palette when injected into web pages, have you considered rendering the React component within a Shadow DOM? This would encapsulate its styles and structure, preventing conflicts with the host page's CSS and JavaScript, and vice-versa.
Mantine UI components might require specific theming or style injection when used inside a Shadow DOM. Here's a conceptual example of how you might set up the root:
// Inside the if (!container) block, after creating the 'container' div:
if (!container) {
container = document.createElement('div');
container.id = containerId;
container.style.zIndex = '2147483647'; // Ensure this is high enough
document.body.appendChild(container);
const shadowRoot = container.attachShadow({ mode: 'open' });
// Create a new div inside the shadow root for React to mount onto
const appRootInShadow = document.createElement('div');
shadowRoot.appendChild(appRootInShadow);
// You'll also need to append styles to the shadowRoot
// For example, by creating a <style> tag or linking Mantine's CSS
// const mantineStyles = document.createElement('style');
// mantineStyles.textContent = `/* Mantine styles here or @import */`;
// shadowRoot.appendChild(mantineStyles);
// Then create the React root on this new div
// const root = ReactDOM.createRoot(appRootInShadow);
// root.render(<App />);
}
// const root = ReactDOM.createRoot(container!); // This line would changeThis approach provides strong encapsulation, which is highly recommended for content script UIs.
| const { keyword, handleChangeKeyword } = useKeywordData(); | ||
| const { bookmarks } = useBookmarkData(keyword); | ||
| const inputRef = useRef<HTMLInputElement | null>(null); | ||
| const linkRefs: any = useRef([]); |
There was a problem hiding this comment.
The linkRefs ref is currently typed as any. To improve type safety and make the component's props and state easier to understand, could we provide a more specific type? Given its usage in BookmarkListItem (which typically involves <a> tags for links), useRef<(HTMLAnchorElement | null)[]>([]) might be a more accurate type. What do you think?
const linkRefs = useRef<(HTMLAnchorElement | null)[]>([]);
Summary
Testing
npm run fmt(fails:romenot found)