fix(frontend): point widget call sites at the renamed markdown helpers - #248
Open
Neilblaze wants to merge 1 commit into
Open
fix(frontend): point widget call sites at the renamed markdown helpers#248Neilblaze wants to merge 1 commit into
Neilblaze wants to merge 1 commit into
Conversation
Signed-off-by: neilblaze <putubanerjee23@gmail.com>
google-oss-prow
Bot
requested review from
chasecadet and
franciscojavierarceo
September 8, 2026 05:42
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
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
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.
Fixes #247
Why
chatbot.jscallsformatMarkdownat:934andescapeHtmlat:1257,:1882and:1883. Neither exists. #234 hoisted both out of theDOMContentLoadedclosure, renamed them toformatChatMarkdownandescapeMarkdownHtml, and converted every call site; atb24399ethe file has no reference to either old name. All four arrive at9a1e5ab(#237).formatMarkdownat:934is on the Stop path, and #242 does not touch it: pressing Stop throws before the interrupted badge and the history push, so the partial answer stays on screen but is never saved and the widget's message state is left uncleared. BecausestopGenerationisasyncand its promise is dropped at:1500, this one surfaces as an unhandled rejection.escapeHtmlcosts more.setToolStatususes it, so the "Searching Kubeflow documentation..." pill #237 added never mounts on any tool-using turn.renderCitationsOnDivuses it too, and it throws inside the SSE handler'scatch (parseError), which swallows it with no console output and beforemessagesHistory.pushandautoSaveCurrentChat()— so the Sources accordion never mounts, and the reply that carried the citations is never written to the saved chat.Nothing in CI can see any of it: the one widget test evaluates only the top-level prelude.
What
chatbot.js—formatMarkdown(->formatChatMarkdown(at:934, andescapeHtml(->escapeMarkdownHtml(at:1257,:1882,:1883. Four tokens, no logic change.tests/test_widget_references.py(new) — scans the whole file for barename(calls and fails on any the file does not declare and that is not a listed browser global.All three
escapeHtmlsites are text content inside a template literal, and none can receivenull/undefined(formatCitationInfodefaults the title,domainis seeded, andsetToolStatus's text is one of eight literals), so the rendered output is identical.escapeMarkdownHtmladditionally escapes"and', which only matters in attribute positions, where the removed helper was unsafe.Verification
Run against every revision of
chatbot.js, the check is clean through #207, #210, #211, #219 and #234, and reports exactly these two names at #237 — PR Safety would have failed on it:pytest: 145 passed, against 143 onmainwith the same toolchainnode --check frontend/docs_scripts/chatbot.jspasses; the five existing widget tests still passruff check/ruff format: no new findings. Both already fail onmain(issue Bug: PR Safety workflow failing on main — ruff format check fails for tests/test_docs_pipeline.py #236, PR fix(ci): satisfy ruff lint and format checks on main #241)Notes for reviewers
Relationship to #242. #242 resolves the same
escapeHtmlReferenceError by adding anescapeHtmlalias, as part of a larger change toformatChatMarkdownthat escapes raw HTML for #175. This points the three call sites at the helper that already exists instead, and also fixesformatMarkdown, which #242 does not touch. #242's hunks are all in the prelude and these are all inside the closure, so the two merge cleanly in either order; if #242 lands first its alias is simply no longer needed by these call sites.Relationship to #240. #240 rewrites the loop around the
catch (parseError)that hides this, though it keeps the samecatch; narrowing it toJSON.parseis worth doing and belongs there or after it. #240 also deletes the one remaining mention offormatMarkdownintests/eval/README.md, so I have deliberately left that line alone rather than create a conflict over a stale doc line.What the check does and does not do
It is a text scan, not a parser: the repo has no JS toolchain, and this needs no Node on the runner. It resolves declarations, classes, method shorthand, arrow and function parameters, skips method calls and regex escapes, and keeps template interpolations in text positions. It does not scan interpolations inside quoted HTML attributes, because quoted strings are blanked first and a single pass that handled both would have to lex regex literals —
/[&<>"']/gin this very file makes that awkward. It resolves names file-wide rather than per scope, so a parameter can mask a same-named call elsewhere, and it only seesname(, so a helper passed by reference is not covered.GLOBALScurrently lists exactly the 16 globals this file uses, with no headroom: adding, say,parseIntorSetfails this test until it is listed. That is deliberate, and the failure message says so, but it is a real cost and worth knowing before you approve it.