Skip to content

Add tab assistant side panel and model metadata overrides#14

Open
samanhappy wants to merge 7 commits into
mainfrom
codex/tab-context-side-panel
Open

Add tab assistant side panel and model metadata overrides#14
samanhappy wants to merge 7 commits into
mainfrom
codex/tab-context-side-panel

Conversation

@samanhappy

Copy link
Copy Markdown
Owner

Summary

  • Add a tab assistant side panel flow with background handling, content-script capture, and global action bar entry point
  • Add tab assistant visibility controls and per-site blacklist settings in the general options form
  • Allow overriding model context window metadata from the model settings UI and persist it in config
  • Include tab assistant copy across supported locales

Testing

  • Not run (not requested)

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a 'Tab Assistant' side panel feature that allows users to chat with and ask questions about the current webpage. It includes background script handlers, a page context extractor utilizing Readability, options to configure model context window overrides, and an IndexedDB database to store chat sessions. The review feedback highlights several critical issues: a potential XSS vulnerability in the markdown link renderer, text duplication in the DOM extractor due to selecting container elements, a message-response race condition when querying frames, an asynchronous race condition when switching tabs rapidly, and unhandled promise rejections when saving messages to collections.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +32 to +43
a: {
component: ({ children, ...props }: any) => (
<a
className="break-words text-blue-600 underline"
target="_blank"
rel="noreferrer"
{...props}
>
{children}
</a>
),
},

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-critical critical

The custom link (a) component override directly spreads props (which includes href) onto the anchor tag without sanitization. If the model-generated markdown contains a malicious javascript: link, clicking it will execute arbitrary JavaScript within the privileged context of the extension's side panel. Sanitize the href attribute to only allow safe protocols (e.g., http:, https:, mailto:, tel:).

        a: {
          component: ({ children, href, ...props }: any) => {
            const isSafe = href && (href.startsWith('http://') || href.startsWith('https://') || href.startsWith('mailto:') || href.startsWith('tel:'));
            return (
              <a
                className="break-words text-blue-600 underline"
                target="_blank"
                rel="noreferrer"
                href={isSafe ? href : undefined}
                {...props}
              >
                {children}
              </a>
            );
          },
        },

Comment thread core/tab-context/extractor.ts Outdated
Comment on lines +14 to +28
const BLOCK_SELECTOR = [
'article',
'main',
'section',
'h1',
'h2',
'h3',
'h4',
'p',
'li',
'blockquote',
'pre',
'td',
'th',
].join(',');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Including container elements like 'article', 'main', and 'section' in BLOCK_SELECTOR causes severe text duplication. When querySelectorAll selects both a container (e.g., <article>) and its children (e.g., <p>), the text content of the children is extracted twice: once as part of the parent container's textContent and again as individual blocks. This wastes the LLM's context window and increases token usage. Remove these high-level container elements from the selector so only actual content-bearing leaf nodes are extracted.

const BLOCK_SELECTOR = [
  'h1',
  'h2',
  'h3',
  'h4',
  'p',
  'li',
  'blockquote',
  'pre',
  'td',
  'th',
].join(',');

Comment thread background.ts Outdated
Comment on lines +533 to +536
const res = await chrome.tabs.sendMessage(tabId, {
action: 'tabContext:capturePage',
budget: request.budget,
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

When sending a message to capture the page context using chrome.tabs.sendMessage(tabId, ...), the message is broadcast to all frames in the tab. Since the content script is injected into all frames, and we explicitly check window.top !== window.self to ignore iframes (returning false immediately), the iframe content scripts will quickly return false and close/respond to the message channel. This can race with the top-level frame's asynchronous response, causing the background script to receive undefined or a failure. Specify { frameId: 0 } in the options to target only the main frame.

          const res = await chrome.tabs.sendMessage(
            tabId,
            {
              action: 'tabContext:capturePage',
              budget: request.budget,
            },
            { frameId: 0 }
          );

Comment on lines +97 to +112
const loadTab = useCallback(async (tab: ActiveTabInfo | null) => {
if (!tab?.id) return;
setActiveTab(tab);
setLoadingContext(true);
setError('');

const captured = await tabContextService.capture(tab.id);
const snapshot = captured || createUnavailableSnapshot(tab);
const normalizedUrl = snapshot.normalizedUrl || normalizePageUrl(tab.url || '');
const nextSession = await tabChatDB.upsertContext(normalizedUrl, snapshot);

setContext(snapshot);
setSession(nextSession);
setMessages(nextSession.messages || []);
setLoadingContext(false);
}, []);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

There is a potential race condition in loadTab. Since loadTab is an asynchronous function, if the user switches tabs rapidly, multiple concurrent calls to tabContextService.capture will be initiated. The responses may resolve out of order, causing the side panel to display the context and messages of a previously active tab instead of the currently active one. Use a useRef to track the ID of the most recently requested tab and ignore the results of stale requests. You will need to define const loadingTabIdRef = useRef<number | null>(null); at the component level.

  const loadTab = useCallback(async (tab: ActiveTabInfo | null) => {
    if (!tab?.id) return;
    loadingTabIdRef.current = tab.id;
    setActiveTab(tab);
    setLoadingContext(true);
    setError('');

    try {
      const captured = await tabContextService.capture(tab.id);
      if (loadingTabIdRef.current !== tab.id) return;

      const snapshot = captured || createUnavailableSnapshot(tab);
      const normalizedUrl = snapshot.normalizedUrl || normalizePageUrl(tab.url || '');
      const nextSession = await tabChatDB.upsertContext(normalizedUrl, snapshot);

      setContext(snapshot);
      setSession(nextSession);
      setMessages(nextSession.messages || []);
    } catch (err: any) {
      if (loadingTabIdRef.current === tab.id) {
        setError(err?.message || labels.error);
      }
    } finally {
      if (loadingTabIdRef.current === tab.id) {
        setLoadingContext(false);
      }
    }
  }, [labels.error]);

Comment on lines +255 to +268
const saveMessageToCollections = async (content: string) => {
if (!content.trim()) return;
await collectService.addItem({
text: content.trim(),
url: context?.url || activeTab?.url || '',
title: context?.title || activeTab?.title || labels.noPageTitle,
hostname: context?.hostname || '',
type: 'page_summary',
source: 'tab_context',
conversation_id: session?.id,
});
setSaveState('saved');
window.setTimeout(() => setSaveState('idle'), 1500);
};

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The saveMessageToCollections function calls the asynchronous collectService.addItem but does not handle potential promise rejections. If the database write fails, it will result in an unhandled promise rejection, and the UI will incorrectly transition to the 'saved' state. Wrap the call in a try-catch block to handle errors gracefully and notify the user.

  const saveMessageToCollections = async (content: string) => {
    if (!content.trim()) return;
    try {
      await collectService.addItem({
        text: content.trim(),
        url: context?.url || activeTab?.url || '',
        title: context?.title || activeTab?.title || labels.noPageTitle,
        hostname: context?.hostname || '',
        type: 'page_summary',
        source: 'tab_context',
        conversation_id: session?.id,
      });
      setSaveState('saved');
      window.setTimeout(() => setSaveState('idle'), 1500);
    } catch (err: any) {
      setError(err?.message || 'Failed to save to collections');
    }
  };

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant