Skip to content

fix(frontend): point widget call sites at the renamed markdown helpers - #248

Open
Neilblaze wants to merge 1 commit into
kubeflow:mainfrom
Neilblaze:fix/widget-undefined-function-refs
Open

fix(frontend): point widget call sites at the renamed markdown helpers#248
Neilblaze wants to merge 1 commit into
kubeflow:mainfrom
Neilblaze:fix/widget-undefined-function-refs

Conversation

@Neilblaze

Copy link
Copy Markdown

Fixes #247

Why

chatbot.js calls formatMarkdown at :934 and escapeHtml at :1257, :1882 and :1883. Neither exists. #234 hoisted both out of the DOMContentLoaded closure, renamed them to formatChatMarkdown and escapeMarkdownHtml, and converted every call site; at b24399e the file has no reference to either old name. All four arrive at 9a1e5ab (#237).

formatMarkdown at :934 is 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. Because stopGeneration is async and its promise is dropped at :1500, this one surfaces as an unhandled rejection.

escapeHtml costs more. setToolStatus uses it, so the "Searching Kubeflow documentation..." pill #237 added never mounts on any tool-using turn. renderCitationsOnDiv uses it too, and it throws inside the SSE handler's catch (parseError), which swallows it with no console output and before messagesHistory.push and autoSaveCurrentChat() — 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.jsformatMarkdown( -> formatChatMarkdown( at :934, and escapeHtml( -> escapeMarkdownHtml( at :1257, :1882, :1883. Four tokens, no logic change.
  • tests/test_widget_references.py (new) — scans the whole file for bare name( calls and fails on any the file does not declare and that is not a listed browser global.

All three escapeHtml sites are text content inside a template literal, and none can receive null/undefined (formatCitationInfo defaults the title, domain is seeded, and setToolStatus's text is one of eight literals), so the rendered output is identical. escapeMarkdownHtml additionally 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:

703c5f2  Feast mcp infra (#207)                       clean
53f1251  feat: 3-tool MCP, TEI embeddings, ...        clean
a02cd19  Updated Branding - ARISE FLO (#211)          clean
80c8e03  feat(gateway): anonymous session-JWT ...     clean
b24399e  fix: stop A2A answers truncating ... (#234)  clean
9a1e5ab  feat(chatbot): surface MCP citations (#237)  ['escapeHtml', 'formatMarkdown']

Notes for reviewers

Relationship to #242. #242 resolves the same escapeHtml ReferenceError by adding an escapeHtml alias, as part of a larger change to formatChatMarkdown that escapes raw HTML for #175. This points the three call sites at the helper that already exists instead, and also fixes formatMarkdown, 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 same catch; narrowing it to JSON.parse is worth doing and belongs there or after it. #240 also deletes the one remaining mention of formatMarkdown in tests/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 — /[&<>"']/g in 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 sees name(, so a helper passed by reference is not covered. GLOBALS currently lists exactly the 16 globals this file uses, with no headroom: adding, say, parseInt or Set fails 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.

Signed-off-by: neilblaze <putubanerjee23@gmail.com>
@google-oss-prow

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign tarekabouzeid for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(frontend): the chat widget calls two functions that no longer exist

1 participant