Skip to content

Feat/ai patterns/source chip#2266

Draft
robertwilde wants to merge 2 commits into
mainfrom
feat/ai-patterns/source-chip
Draft

Feat/ai patterns/source chip#2266
robertwilde wants to merge 2 commits into
mainfrom
feat/ai-patterns/source-chip

Conversation

@robertwilde

Copy link
Copy Markdown
Collaborator

No description provided.

@robertwilde robertwilde force-pushed the feat/ai-patterns/source-chip branch from 5b5ace3 to d7aa7ea Compare July 2, 2026 15:39

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

Copy link
Copy Markdown
Contributor

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 inline and summary source chips (SiSourceChipInlineComponent and SiSourceChipSummaryComponent) for AI messages, allowing citations to be displayed either inline at the end of the text or as a summary alongside action buttons. The review feedback points out two correctness issues in the DOM manipulation logic of SiAiMessageComponent: a caching bug that prevents dynamic updates of inline chips (e.g., during streaming), and a layout bug where chips are incorrectly appended if the message does not end with a paragraph element. A code suggestion is provided to resolve both issues.

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 +119 to +142
if (inlineEl) {
// Collect projected element nodes on the first Node render.
if (this.inlineNodes.length === 0) {
this.inlineNodes = Array.from(inlineEl.childNodes).filter(
n => n.nodeType === Node.ELEMENT_NODE
);
}
// Move chips to the end of the last paragraph in the rendered content.
if (this.inlineNodes.length > 0) {
const allPs = container.querySelectorAll('p');
const target = allPs.length > 0 ? allPs[allPs.length - 1] : container;
// Prepend a 6px inline-block spacer. Because it is a separate DOM node it
// stays on the previous line when the chip wraps, so the chip starts at
// position 0 rather than carrying a 6px indent onto the new line.
if (!this.chipSpacer) {
const spacer = document.createElement('span');
spacer.style.cssText = 'display:inline-block;width:6px';
spacer.setAttribute('aria-hidden', 'true');
this.chipSpacer = spacer;
}
target.appendChild(this.chipSpacer);
this.inlineNodes.forEach(node => target.appendChild(node));
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

There are two correctness issues in this block:

  1. Dynamic Update Bug: The check 'if (this.inlineNodes.length === 0)' prevents re-collecting projected nodes when they are dynamically added or updated (e.g., during AI streaming or lazy loading). Since 'this.inlineNodes' is cached on the first run, any subsequent additions of chips will be left behind in 'inlineEl' and never moved to the target.
  2. Non-paragraph End Element Bug: Querying for all paragraphs and taking the last one ('allPs[allPs.length - 1]') assumes the message always ends with a paragraph. If the message ends with a list ('
      '), code block ('
      '), or table, the chip will be appended to the last paragraph before that element, which breaks the visual order. We should check if the last child of the container is a '

      ' and append to it, otherwise append directly to the container.

            if (inlineEl) {
              // Collect projected element nodes.
              this.inlineNodes = Array.from(inlineEl.childNodes).filter(
                n => n.nodeType === Node.ELEMENT_NODE
              );
              // Move chips to the end of the last paragraph in the rendered content if it ends with one, otherwise append to the container.
              if (this.inlineNodes.length > 0) {
                const lastChild = container.lastElementChild;
                const target = lastChild && lastChild.tagName.toLowerCase() === 'p' ? lastChild : container;
                // Prepend a 6px inline-block spacer. Because it is a separate DOM node it
                // stays on the previous line when the chip wraps, so the chip starts at
                // position 0 rather than carrying a 6px indent onto the new line.
                if (!this.chipSpacer) {
                  const spacer = document.createElement('span');
                  spacer.style.cssText = 'display:inline-block;width:6px';
                  spacer.setAttribute('aria-hidden', 'true');
                  this.chipSpacer = spacer;
                }
                target.appendChild(this.chipSpacer);
                this.inlineNodes.forEach(node => target.appendChild(node));
              }
            }

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