refactor(ai-assistant): changed sources section style - #702
Draft
davids-ensemble wants to merge 3 commits into
Draft
refactor(ai-assistant): changed sources section style#702davids-ensemble wants to merge 3 commits into
davids-ensemble wants to merge 3 commits into
Conversation
|
Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch and validate page speed.
|
❌ Test ResultsStatus: Some tests failed! 🔍 Click to view failed testsTest Coverage ReportOverall Coverage Summary
Coverage by File/Directory
Coverage report generated at 2026-07-14T14:23:08.273Z |
davids-ensemble
force-pushed
the
feat-ai-assistant-sources-rework
branch
from
August 5, 2026 16:14
c0b9eb5 to
51afcfd
Compare
aem-code-sync
Bot
temporarily deployed
to
feat-ai-assistant-sources-rework
August 5, 2026 16:14
Inactive
❌ Test ResultsStatus: Some tests failed! 🔍 Click to view failed testsTest Coverage ReportOverall Coverage Summary
Coverage by File/Directory
Coverage report generated at 2026-08-05T16:15:02.778Z |
Citation events can arrive mid-stream, and appendReferences was being called from onCitation, which made the sources section flash in before the response finished streaming. References are now accumulated during streaming and rendered once in onComplete.
aem-code-sync
Bot
temporarily deployed
to
feat-ai-assistant-sources-rework
August 5, 2026 16:15
Inactive
❌ Test ResultsStatus: Some tests failed! 🔍 Click to view failed testsTest Coverage ReportOverall Coverage Summary
Coverage by File/Directory
Coverage report generated at 2026-08-05T16:16:10.514Z |
❌ Test ResultsStatus: Some tests failed! 🔍 Click to view failed testsTest Coverage ReportOverall Coverage Summary
Coverage by File/Directory
Coverage report generated at 2026-08-05T16:18:22.662Z |
davids-ensemble
marked this pull request as ready for review
August 5, 2026 16:19
louisachu
self-requested a review
August 10, 2026 16:29
louisachu
reviewed
Aug 10, 2026
Collaborator
There was a problem hiding this comment.
Code Review
All in hlx_statics/blocks/ai-assistant/ai-assistant_chat-bubble.js.
Must fix
- L464
const host = new URL(url).host;— a single bad URL throws and no sources render → wrap in try/catch. - L483
daa-llvalue...Link:${a.textContent}|${url}—:in the title/URL breaks analytics parsing → sanitize/shorten.
Should fix
- L465 favicon
<img>has noalt/onerror→ addalt=""and hide on load error. - L430–431 / L457–458 ids become
...-undefinedwhenthis.idis null → add a fallback id.
Nice to have
- L495
sourcesRegion.hidden = newState === false;→ simpler as!newState.
| if (references?.length) { | ||
| // Accumulate references but defer rendering until the response | ||
| // finishes streaming (see onComplete). | ||
| accumulatedReferences = references; |
Collaborator
There was a problem hiding this comment.
Named accumulatedReferences / commented "Accumulate," but this replaces. If onCitation fires more than once with disjoint payloads, only the last batch renders. Does it fire more than once per response?
Collaborator
Author
There was a problem hiding this comment.
I think the references event only fires once. I delayed when we show them because the browser limits how often you can update an element which lead to situations where you would get the sources section before the bubble with the response finished rendering all the content.
In the future I also intend to improve the response streaming/parsing part so we are not hitting the browser so hard which will also limit the amount of updates that the bubble receives.
I guess the comment is a bit confusing in this case so I'll update it.
| button.addEventListener("click", () => { | ||
| const isExpanded = button.ariaExpanded === "true"; | ||
| const newState = !isExpanded; | ||
| sourcesRegion.hidden = newState === false; |
There was a problem hiding this comment.
can this be written as !newState instead of newState === false
| a.setAttribute( | ||
| "daa-ll", | ||
| `DevsiteAI Assistant:Message:Sources:Link:${a.textContent}|${url}`, | ||
| ); |
There was a problem hiding this comment.
daa-ll analytics attribute now embeds raw title + URL. If a title contains either :/| of character (common in doc titles), it can corrupt how downstream analytics parses that field.
| const list = createTag("ol", { class: "chat-bubble-sources-list" }); | ||
| references.forEach(({ url, title }) => { | ||
| references.forEach(({ url, title }, idx) => { | ||
| const host = new URL(url).host; |
There was a problem hiding this comment.
Unguarded new URL(url).host — if any reference URL is ever malformed or relative, this throws inside the forEach and aborts the rest of appendReferences (and everything chained after it in onComplete) with no try/catch.
| button.ariaExpanded = String(newState); | ||
| buttonIndicator.classList.toggle("rotated"); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Quick question on the sources rendering change — looks like we moved from showing sources as soon as a citation comes in, to only rendering them once onComplete fires. What happens if the stream errors out after onCitation has already set accumulatedReferences, but before onComplete gets a chance to run? Since onError doesn't call appendReferences, wouldn't the sources we already got from the backend just get dropped in that case? Previously they'd have shown up immediately, so this feels like it could be a regression on the error path — might be worth appending whatever we've accumulated in onError too, unless I'm missing something.
davids-ensemble
marked this pull request as draft
August 12, 2026 06:00
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.