Feat/ai patterns/source chip#2266
Conversation
5b5ace3 to
d7aa7ea
Compare
There was a problem hiding this comment.
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.
| 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)); | ||
| } | ||
| } |
There was a problem hiding this comment.
There are two correctness issues in this block:
- 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.
- 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));
}
}
No description provided.