feat(chat): citations & sources visual + interaction system#769
Conversation
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ive pointer detection
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Claude finished @blove's task in 3m 28s —— View job Code Review
Overall the PR is well-structured. The display helpers are pure and well-tested, the pointer-adaptive interaction model is sound, and the tap-gesture regression test ( Bugs
Accessibility
Minor (non-blocking)
Public API surface looks good — the three new |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| } | ||
| } | ||
|
|
||
| protected onAttached(pane: HTMLElement): void { |
There was a problem hiding this comment.
Bug: event listeners are added here but never removed.
close() sets this.pane = null without first calling removeEventListener. If the overlay directive reuses the same portal element across open/close cycles, each cycle adds two more listeners to the same node. Even when the element is freshly created each time, the closures hold a reference back to the component and can delay GC.
The fix is to remove the listeners before clearing the reference:
| protected onAttached(pane: HTMLElement): void { | |
| protected onAttached(pane: HTMLElement): void { | |
| this.pane = pane; | |
| pane.addEventListener('mouseenter', this.onPaneEnter); | |
| pane.addEventListener('mouseleave', this.onPaneLeave); | |
| } |
…and in close():
protected close(): void {
this.clearTimers();
this.justOpenedByFocus = false;
this.open.set(false);
if (this.pane) {
this.pane.removeEventListener('mouseenter', this.onPaneEnter);
this.pane.removeEventListener('mouseleave', this.onPaneLeave);
this.pane = null;
}
}Also extend the DestroyRef.onDestroy callback to do the same cleanup so a component that is destroyed while the preview is open doesn't orphan listeners.
| this.justOpenedByFocus = true; | ||
| } | ||
|
|
||
| protected onBlur(): void { |
There was a problem hiding this comment.
Reliability: prefer event.relatedTarget over document.activeElement in onBlur.
During a blur → focus transition, document.activeElement can transiently be document.body (or even null) in several browsers before the new element fully receives focus. This means this.pane.contains(active) can return false even when focus is genuinely moving into the pane, causing a spurious close.
FocusEvent.relatedTarget is the element that just received focus — it is set synchronously and is more reliable for this check:
| protected onBlur(): void { | |
| protected onBlur(e: FocusEvent): void { | |
| this.justOpenedByFocus = false; | |
| const relatedTarget = e.relatedTarget as Element | null; | |
| if (this.pane && relatedTarget && this.pane.contains(relatedTarget)) return; | |
| this.close(); | |
| } |
| [attr.rel]="r.citation.url ? 'noopener noreferrer' : null" | ||
| [attr.role]="r.citation.url ? null : 'button'" | ||
| [attr.tabindex]="r.citation.url ? null : '0'" | ||
| aria-haspopup="dialog" |
There was a problem hiding this comment.
ARIA semantic mismatch: aria-haspopup="dialog" but the popup has role="group".
aria-haspopup="dialog" tells assistive technology that activating this element opens a dialog, which implies focus trapping and an explicit close mechanism. The preview card uses role="group" (or no explicit role), which is not a dialog. Screen readers that honour aria-haspopup="dialog" will misrepresent the interaction.
For a tooltip-like hover/tap preview with no focus trap, two correct options:
- Drop
aria-haspopupentirely and rely onaria-expandedto convey state —aria-expandedalone is sufficient for a disclosure pattern. - Keep the popup as a
role="tooltip"(on the preview element) and switch the marker attribute toaria-describedbypointing at the tooltip'sid.
| import type { Citation } from '../../agent/citation'; | ||
| import { deriveDomain, deriveMonogram, monogramColor, citationTypeLabel } from '../../agent/citation-display'; | ||
| import { CHAT_HOST_TOKENS } from '../../styles/chat-tokens'; | ||
| import { CHAT_CITATIONS_PANEL_STYLES } from '../../styles/chat-citations.styles'; |
There was a problem hiding this comment.
Style bloat: card component imports the full panel stylesheet.
CHAT_CITATIONS_PANEL_STYLES contains two unrelated rule-sets — the panel header/list selectors (.chat-citations__*) and the card selectors (.chat-citations-card*). ChatCitationsCardComponent only needs the card rules, but it injects the entire string into its shadow DOM. Since Angular component styles are scoped by the view encapsulation emulation prefix, none of the panel selectors will match anything in the card's template, but they add unnecessary bytes to every card instance's encapsulated stylesheet.
Consider splitting the export into CHAT_CITATIONS_PANEL_STYLES (header + list) and CHAT_CITATIONS_CARD_STYLES (card rules only), so each component imports only what it uses.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Claude finished @blove's task in 3m 56s —— View job Code Review
The PR is well-structured overall — display helpers are pure and well-tested, the pointer-adaptive model is sound, and the tap-gesture regression test is a good explicit contract. Five issues need addressing before merge; three of them are the same bugs flagged in the prior automated review and remain unaddressed in the current HEAD. Bugs (blocking)
Accessibility (blocking)
Minor (non-blocking)
Public API surface looks good — the three new |
| this.justOpenedByFocus = false; | ||
| this.open.set(false); | ||
| this.pane = null; // directive removes the pane element on detach | ||
| } |
There was a problem hiding this comment.
Bug (still open from prior review): event listener leak.
close() nulls this.pane without first calling removeEventListener, so the mouseenter/mouseleave handlers accumulate on the pane element across open/close cycles. The DestroyRef.onDestroy callback at construction time also only calls clearTimers() — if the component is destroyed while the overlay is open, the listeners are orphaned.
| } | |
| protected close(): void { | |
| this.clearTimers(); | |
| this.justOpenedByFocus = false; | |
| this.open.set(false); | |
| if (this.pane) { | |
| this.pane.removeEventListener('mouseenter', this.onPaneEnter); | |
| this.pane.removeEventListener('mouseleave', this.onPaneLeave); | |
| this.pane = null; | |
| } | |
| } |
And update the DestroyRef.onDestroy callback in the constructor:
inject(DestroyRef).onDestroy(() => {
this.clearTimers();
if (this.pane) {
this.pane.removeEventListener('mouseenter', this.onPaneEnter);
this.pane.removeEventListener('mouseleave', this.onPaneLeave);
}
});
| const active = this.document.activeElement; | ||
| if (this.pane && active && this.pane.contains(active)) return; // focus moved into card | ||
| this.close(); | ||
| } |
There was a problem hiding this comment.
Bug (still open from prior review): document.activeElement is unreliable in onBlur.
During a blur→focus transition, document.activeElement can transiently be document.body before the next element receives focus, causing the pane-contains check to return false and spuriously closing the preview when the user tabs into it.
FocusEvent.relatedTarget is set synchronously to the element receiving focus and is reliable. The template binding also needs to pass $event:
Template fix (line 51): change (blur)="onBlur()" → (blur)="onBlur($event)"
| } | |
| protected onBlur(e: FocusEvent): void { | |
| this.justOpenedByFocus = false; | |
| const relatedTarget = e.relatedTarget as Element | null; | |
| if (this.pane && relatedTarget && this.pane.contains(relatedTarget)) return; | |
| this.close(); | |
| } |
| [attr.rel]="r.citation.url ? 'noopener noreferrer' : null" | ||
| [attr.role]="r.citation.url ? null : 'button'" | ||
| [attr.tabindex]="r.citation.url ? null : '0'" | ||
| aria-haspopup="dialog" |
There was a problem hiding this comment.
Accessibility (still open from prior review): aria-haspopup="dialog" doesn't match the popup.
The preview card uses role="group", not role="dialog". Screen readers that honour aria-haspopup="dialog" expect focus to be trapped inside a dialog with an explicit close action; a hover/focus preview card is not that. aria-expanded alone is sufficient for this disclosure pattern.
| aria-haspopup="dialog" | |
| [attr.aria-expanded]="open()" |
| import type { Citation } from '../../agent/citation'; | ||
| import { deriveDomain, deriveMonogram, monogramColor, citationTypeLabel } from '../../agent/citation-display'; | ||
| import { CHAT_HOST_TOKENS } from '../../styles/chat-tokens'; | ||
| import { CHAT_CITATIONS_PANEL_STYLES } from '../../styles/chat-citations.styles'; |
There was a problem hiding this comment.
Minor (still open from prior review): card component imports the full panel stylesheet.
CHAT_CITATIONS_PANEL_STYLES contains both the panel header/list selectors (.chat-citations__*) and the card selectors (.chat-citations-card*). The card only needs the latter, but injects the full string. Splitting the export into CHAT_CITATIONS_PANEL_STYLES and CHAT_CITATIONS_CARD_STYLES removes unused rules from every card's encapsulated stylesheet.
| border-radius: var(--tplane-chat-radius-card); | ||
| text-decoration: none; color: inherit; | ||
| cursor: pointer; | ||
| transition: border-color 120ms ease, background 120ms ease; |
There was a problem hiding this comment.
Minor: cursor: pointer applies to the non-interactive no-URL <div> card.
.chat-citations-card { cursor: pointer } is shared by both the <a> (url present) and the <div> (no url) variants rendered in ChatCitationsCardComponent. The <div> has no click handler and no interactive role, so pointer misleads users.
| transition: border-color 120ms ease, background 120ms ease; | |
| .chat-citations-card { | |
| display: flex; gap: 10px; | |
| padding: 10px 11px; | |
| background: var(--tplane-chat-surface); | |
| border: 1px solid var(--tplane-chat-separator); | |
| border-radius: var(--tplane-chat-radius-card); | |
| text-decoration: none; color: inherit; | |
| transition: border-color 120ms ease, background 120ms ease; | |
| } | |
| a.chat-citations-card { cursor: pointer; } |
1 participant
Summary
Gives
@threadplane/chata prod-ready, cohesive, extensible visual + interaction system for citations and sources — previously the plumbing existed (semantic BEM markup, resolver, two-tier provider/markdown citations) but had zero styling. Now:role=button, preview-only), and unresolved (muted dashed, non-interactive).chatConnectedOverlayprimitive: favicon/monogram, domain, source-type, title, snippet, an explicit Open source action, and a freshness slot. "Lightweight by default, deep on demand."Sources · Nheader (stacked-favicon preview + chevron) over detail cards sharing the preview card's visual language; expanded by default, scales from 2 to 20+ sources.Everything is driven by new
--tplane-chat-citation-*tokens (light/dark for free), browser-safe (no third-party favicon fetches — monogram fallback; consumers can supplyiconUrl), accessible (keyboard, focus, WCAG-AA contrast, reduced-motion), and pointer-adaptive (hover on desktop, tap on touch — no surprise navigation).Design spec:
docs/superpowers/specs/2026-07-06-chat-citations-sources-design.mdPlan:
docs/superpowers/plans/2026-07-06-chat-citations-sources.mdPublic API
Citationgains optionalsourceType?,iconUrl?,publishedAt?(non-breaking).ChatCitationPreviewComponent; pure display helpers (deriveDomain,deriveMonogram,monogramColor,citationTypeLabel,formatPublished, …) exported for customchatCitationCardtemplates.Test Plan
npx nx test chat(951 tests): marker states, preview-card content, detail card, panel collapse, display helpers, tokens, and a tap-gesture regression test (preview must not self-close on focus→click).npx nx lint chat),npx nx build chatsucceeds.strict:falseand loads with a clean console (examples-chat).examples-chat, run welcome suggestion feat: glassy gradient website redesign + docs refresh #3 ("search + cite inline[^id]"), then verify in a real browser: pills render (incl. grouped/in-table), hover/focus opens the preview and moving into it keeps it open, click opens the source, Sources panel collapses/expands, dark mode contrast, and mobile tap previews without navigating.Reviewer notes
A final adversarial review caught and this PR fixes: tap/Enter self-closing the preview (focus→click race), a WCAG-AA contrast miss on muted text, non-reactive pointer detection (SSR), a dead
--no-urlclass, and cross-file duplication (extracted to shared helpers).🤖 Generated with Claude Code