Skip to content

feat(chat-messages): add citation summary#2143

Draft
robertwilde wants to merge 5 commits into
mainfrom
feat/ai/summary-pill
Draft

feat(chat-messages): add citation summary#2143
robertwilde wants to merge 5 commits into
mainfrom
feat/ai/summary-pill

Conversation

@robertwilde

Copy link
Copy Markdown
Collaborator

Related to #2064

Depends on #2099 and #2126 to be merged first.

Introduce a new source citation feature for chat messages. The first commit adds a SiCitationButton component to si-ai-message, a standalone button that opens a popover displaying source citations, with its own styles, unit tests, and updated API golden. The second commit adds E2E snapshot coverage for the new citation button and its popover in both si-ai-message and si-chat-container examples, along with a helper update in test-helpers.ts to support the new snapshot scenarios.

@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 support for inline and summary citations in AI chat messages by adding the SiCitationPillComponent and SiCitationButtonComponent, along with the corresponding SiChatAnnotatedText data models, examples, and Playwright tests. The review feedback highlights several critical areas for improvement: resolving potential memory leaks and SSR errors in the new components by properly cleaning up CDK overlay event listeners and timeouts; fixing a bug where $event was omitted from a click handler, preventing navigation cancellation; addressing accessibility concerns such as removing forced line breaks, avoiding template-based text truncation, and removing redundant ARIA roles; and ensuring compliance with the repository's UX writing style guide regarding button capitalization and punctuation.

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 +76 to +86
@if (cit.url) {
<a
class="link-text d-inline-block mt-3"
target="_blank"
rel="noopener noreferrer"
[href]="cit.url"
(click)="onCitationClicked(cit)"
>{{ viewSource | translate }}
<i aria-hidden="true" class="link-icon element-right-2 flip-rtl"></i>
</a>
}

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

In the (click) handler of the anchor link, the $event object is not passed to onCitationClicked(cit). As a result, event?.preventDefault() inside the component method is called with undefined, and the default browser navigation (opening the link in a new tab) is not prevented. Pass $event to ensure custom handling works as documented.

Suggested change
@if (cit.url) {
<a
class="link-text d-inline-block mt-3"
target="_blank"
rel="noopener noreferrer"
[href]="cit.url"
(click)="onCitationClicked(cit)"
>{{ viewSource | translate }}
<i aria-hidden="true" class="link-icon element-right-2 flip-rtl"></i>
</a>
}
@if (cit.url) {
<a
class="link-text d-inline-block mt-3"
target="_blank"
rel="noopener noreferrer"
[href]="cit.url"
(click)="onCitationClicked(cit, $event)"
>{{ viewSource | translate }}
<i aria-hidden="true" class="link-icon element-right-2 flip-rtl"></i>
</a>
}

Comment on lines +7 to +14
} @else if (!showSourceCitationButton()) {
@let cit = getCitation(segment.citationId);
<si-citation-pill
[citation]="cit"
[label]="cit.title.length > 24 ? cit.title.slice(0, 24) + '…' : cit.title"
(clicked)="citationClicked.emit($event)"
/><br />
}

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.

medium

The <br /> tag immediately following the <si-citation-pill> component forces a line break, which prevents the citation pills from rendering inline as intended. Removing this <br /> allows the text and citation pills to flow naturally within the paragraph.

Additionally, truncating the citation title in the template using .slice(0, 24) + '…' is an accessibility anti-pattern because screen readers will read the truncated text instead of the full title. It is better to pass the full title to the label and handle visual truncation via CSS (max-width, text-overflow: ellipsis, overflow: hidden) so that the full text remains accessible in the DOM.

Suggested change
} @else if (!showSourceCitationButton()) {
@let cit = getCitation(segment.citationId);
<si-citation-pill
[citation]="cit"
[label]="cit.title.length > 24 ? cit.title.slice(0, 24) + '…' : cit.title"
(clicked)="citationClicked.emit($event)"
/><br />
}
} @else if (!showSourceCitationButton()) {
@let cit = getCitation(segment.citationId);
<si-citation-pill
[citation]="cit"
[label]="cit.title"
(clicked)="citationClicked.emit($event)"
/>
}

Comment on lines +105 to +110
private readonly elementRef = inject(ElementRef);
private readonly document = inject(DOCUMENT);
private readonly destroyRef = inject(DestroyRef);
private hideTimer: ReturnType<typeof setTimeout> | undefined;
private overlayListenersAttached = false;
private observer: MutationObserver | undefined;

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.

medium

The event listeners added to the CDK overlay panel are never removed, which can lead to memory leaks if the component is destroyed while the popover is active or if the panel is reused. Additionally, the setTimeout callback in attachOverlayHoverListeners is not cleared upon component destruction, which could attempt to run code on a destroyed component. Use an AbortController to cleanly remove the event listeners and store the timeout ID to clear it in onDestroy. Also, ensure browser-dependent APIs are only executed in a browser environment using isPlatformBrowser to prevent SSR errors.

Suggested change
private readonly elementRef = inject(ElementRef);
private readonly document = inject(DOCUMENT);
private readonly destroyRef = inject(DestroyRef);
private hideTimer: ReturnType<typeof setTimeout> | undefined;
private overlayListenersAttached = false;
private observer: MutationObserver | undefined;
private readonly elementRef = inject(ElementRef);
private readonly document = inject(DOCUMENT);
private readonly destroyRef = inject(DestroyRef);
private readonly platformId = inject(PLATFORM_ID);
private hideTimer: ReturnType<typeof setTimeout> | undefined;
private hoverListenerTimeout: ReturnType<typeof setTimeout> | undefined;
private overlayAbortController: AbortController | undefined;
private overlayListenersAttached = false;
private observer: MutationObserver | undefined;
References
  1. When using browser-dependent UI features like Angular CDK Overlay, ensure they are only executed in a browser environment. Use a check like isPlatformBrowser to prevent errors during Server-Side Rendering (SSR).

Comment on lines +137 to +142
ngOnInit(): void {
this.destroyRef.onDestroy(() => {
clearTimeout(this.hideTimer);
this.observer?.disconnect();
});
}

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.

medium

Clean up the newly introduced hoverListenerTimeout and the overlay event listeners using the AbortController when the component is destroyed.

Suggested change
ngOnInit(): void {
this.destroyRef.onDestroy(() => {
clearTimeout(this.hideTimer);
this.observer?.disconnect();
});
}
ngOnInit(): void {
this.destroyRef.onDestroy(() => {
clearTimeout(this.hideTimer);
clearTimeout(this.hoverListenerTimeout);
this.cleanupOverlayListeners();
});
}

Comment on lines +162 to +183
private attachOverlayHoverListeners(): void {
if (this.overlayListenersAttached) return;
setTimeout(() => {
const dir = this.popoverDir();
if (!dir) return;
const panel = this.document.getElementById(dir.popoverId)?.closest('.cdk-overlay-pane');
if (!panel) return;
this.overlayListenersAttached = true;
const onEnter = (): void => clearTimeout(this.hideTimer);
const onLeave = (): void => this.scheduleHide();
panel.addEventListener('mouseenter', onEnter);
panel.addEventListener('mouseleave', onLeave);
this.observer = new MutationObserver(() => {
if (!this.document.contains(panel)) {
this.overlayListenersAttached = false;
this.observer?.disconnect();
this.observer = undefined;
}
});
this.observer.observe(this.document.body, { childList: true, subtree: true });
});
}

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.

medium

Refactor attachOverlayHoverListeners to use the AbortController for safe event listener removal and store the timeout ID. Also, add a helper method cleanupOverlayListeners to handle the cleanup. Ensure that these browser-dependent operations are wrapped in an isPlatformBrowser check to avoid errors during Server-Side Rendering (SSR).

  private attachOverlayHoverListeners(): void {
    if (!isPlatformBrowser(this.platformId)) return;
    if (this.overlayListenersAttached) return;
    this.hoverListenerTimeout = setTimeout(() => {
      const dir = this.popoverDir();
      if (!dir) return;
      const panel = this.document.getElementById(dir.popoverId)?.closest('.cdk-overlay-pane');
      if (!panel) return;
      this.overlayListenersAttached = true;
      this.overlayAbortController = new AbortController();
      const onEnter = (): void => clearTimeout(this.hideTimer);
      const onLeave = (): void => this.scheduleHide();
      panel.addEventListener('mouseenter', onEnter, { signal: this.overlayAbortController.signal });
      panel.addEventListener('mouseleave', onLeave, { signal: this.overlayAbortController.signal });
      this.observer = new MutationObserver(() => {
        if (!this.document.contains(panel)) {
          this.cleanupOverlayListeners();
        }
      });
      this.observer.observe(this.document.body, { childList: true, subtree: true });
    });
  }

  private cleanupOverlayListeners(): void {
    this.overlayListenersAttached = false;
    this.overlayAbortController?.abort();
    this.overlayAbortController = undefined;
    this.observer?.disconnect();
    this.observer = undefined;
  }
References
  1. When using browser-dependent UI features like Angular CDK Overlay, ensure they are only executed in a browser environment. Use a check like isPlatformBrowser to prevent errors during Server-Side Rendering (SSR).

Comment on lines +180 to +202
private attachOverlayHoverListeners(): void {
if (this.overlayListenersAttached) return;
// Run after the overlay is attached to the DOM
setTimeout(() => {
const dir = this.popoverDir();
if (!dir) return;
const panel = this.document.getElementById(dir.popoverId)?.closest('.cdk-overlay-pane');
if (!panel) return;
this.overlayListenersAttached = true;
const onEnter = (): void => clearTimeout(this.hideTimer);
const onLeave = (): void => this.scheduleHide();
panel.addEventListener('mouseenter', onEnter);
panel.addEventListener('mouseleave', onLeave);
// Reset flag once the panel is removed from the DOM
this.observer = new MutationObserver(() => {
if (!this.document.contains(panel)) {
this.overlayListenersAttached = false;
this.observer?.disconnect();
this.observer = undefined;
}
});
this.observer.observe(this.document.body, { childList: true, subtree: true });
});

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.

medium

Refactor attachOverlayHoverListeners to use the AbortController for safe event listener removal and store the timeout ID. Also, add a helper method cleanupOverlayListeners to handle the cleanup. Ensure that these browser-dependent operations are wrapped in an isPlatformBrowser check to avoid errors during Server-Side Rendering (SSR).

  private attachOverlayHoverListeners(): void {
    if (!isPlatformBrowser(this.platformId)) return;
    if (this.overlayListenersAttached) return;
    // Run after the overlay is attached to the DOM
    this.hoverListenerTimeout = setTimeout(() => {
      const dir = this.popoverDir();
      if (!dir) return;
      const panel = this.document.getElementById(dir.popoverId)?.closest('.cdk-overlay-pane');
      if (!panel) return;
      this.overlayListenersAttached = true;
      this.overlayAbortController = new AbortController();
      const onEnter = (): void => clearTimeout(this.hideTimer);
      const onLeave = (): void => this.scheduleHide();
      panel.addEventListener('mouseenter', onEnter, { signal: this.overlayAbortController.signal });
      panel.addEventListener('mouseleave', onLeave, { signal: this.overlayAbortController.signal });
      // Reset flag once the panel is removed from the DOM
      this.observer = new MutationObserver(() => {
        if (!this.document.contains(panel)) {
          this.cleanupOverlayListeners();
        }
      });
      this.observer.observe(this.document.body, { childList: true, subtree: true });
    });
  }

  private cleanupOverlayListeners(): void {
    this.overlayListenersAttached = false;
    this.overlayAbortController?.abort();
    this.overlayAbortController = undefined;
    this.observer?.disconnect();
    this.observer = undefined;
  }
References
  1. When using browser-dependent UI features like Angular CDK Overlay, ensure they are only executed in a browser environment. Use a check like isPlatformBrowser to prevent errors during Server-Side Rendering (SSR).

SiPopoverBodyDirective
],
template: `
<span role="group" [siPopover]="citationPreview" [siPopoverPlacement]="placement()">

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.

medium

The span element wraps a single interactive element (either an <a> or a <button>). Applying role="group" to a wrapper of a single control is an accessibility anti-pattern and can confuse screen readers. Remove role="group" from the span.

Suggested change
<span role="group" [siPopover]="citationPreview" [siPopoverPlacement]="placement()">
<span [siPopover]="citationPreview" [siPopoverPlacement]="placement()">

Comment on lines +27 to +28
'SI_CITATION_PILL.LABEL'?: string;
'SI_CITATION_PILL.VIEW_SOURCE'?: string;

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.

medium

The SiCitationButtonComponent uses SI_CITATION_BUTTON.LABEL as its default label, but this key is missing from the SiTranslatableKeys interface. Add 'SI_CITATION_BUTTON.LABEL'?: string; to ensure type safety and completeness of translatable keys.

Suggested change
'SI_CITATION_PILL.LABEL'?: string;
'SI_CITATION_PILL.VIEW_SOURCE'?: string;
'SI_CITATION_PILL.LABEL'?: string;
'SI_CITATION_PILL.VIEW_SOURCE'?: string;
'SI_CITATION_BUTTON.LABEL'?: string;

[title]="showSourceCitationButton() ? 'Switch to inline citations' : 'Switch to summary citation'"
(click)="showSourceCitationButton.set(!showSourceCitationButton())"
>
{{ showSourceCitationButton() ? 'Use Inline Citations' : 'Use Summary Citation' }}

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.

medium

The button labels "Use Inline Citations" and "Use Summary Citation" violate the repository style guide's capitalization rules. According to the style guide, you must "Capitalize only the first letter of the first word in titles, tooltips, menu items, list items, and buttons". Change them to "Use inline citations" and "Use summary citation".

Suggested change
{{ showSourceCitationButton() ? 'Use Inline Citations' : 'Use Summary Citation' }}
{{ showSourceCitationButton() ? 'Use inline citations' : 'Use summary citation' }}
References
  1. Capitalize only the first letter of the first word in titles, tooltips, menu items, list items, and buttons. (link)

[checked]="showSourceCitationButton()"
(change)="showSourceCitationButton.set(!showSourceCitationButton())"
/>
Toggle source citation.

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.

medium

The label "Toggle source citation." contains a trailing full stop. According to the repository style guide, you should "Avoid full sentences on interactive UI pages — use telegram style without a trailing full stop". Remove the trailing period.

Suggested change
Toggle source citation.
Toggle source citation
References
  1. Avoid full sentences on interactive UI pages — use telegram style without a trailing full stop. (link)

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