Skip to content

fix: render button tooltips through CardBody once - #499

Open
dungdong-aws wants to merge 3 commits into
Amazon-Q-Developer:mainfrom
dungdong-aws:fix/button-tooltip-double-parse
Open

dungdong-aws wants to merge 3 commits into
Amazon-Q-Developer:mainfrom
dungdong-aws:fix/button-tooltip-double-parse

Conversation

@dungdong-aws

@dungdong-aws dungdong-aws commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Problem

Icon-only buttons in the top navigation bar — including MCP and chat history — opened a tooltip bubble with no usable text.

Screenshots

Before

Screenshot 2026-09-15 at 4 36 02 PM Screenshot 2026-09-15 at 4 36 13 PM

After

Screenshot 2026-09-15 at 4 36 39 PM Screenshot 2026-09-15 at 4 36 46 PM

Root cause

Button tooltips were parsed twice:

  1. Button converted the label/tooltip markdown to HTML with parseMarkdown(...).
  2. Button passed that generated HTML to CardBody, which owns markdown rendering and parsed it again.

This double parse was latent from the original Button tooltip implementation (1d733b7c, March 2024). It remained harmless while marked passed raw HTML through, including when the MCP/history descriptions were present in July 2025 with mynah-ui 4.36.2.

The XSS hardening in #484 / 571d6c25 correctly began escaping all raw HTML. From mynah-ui 4.40.2 onward, the second parse therefore treated the first parse's generated <p>...</p> as untrusted HTML, exposing the latent defect. The security protection should remain unchanged.

Fix

Make CardBody the single owner of markdown rendering. Button now assembles and passes raw label/tooltip markdown instead of pre-rendered HTML.

- tooltipText += parseMarkdown(props.tooltip ?? '', { includeLineBreaks: true });
+ tooltipText += props.tooltip;

This preserves markdown formatting while avoiding the security-hardened second parse.

Tests

Added focused coverage for both affected paths:

  • explicit tooltip markdown on an icon-only button
  • automatically generated tooltip markdown for a truncated button label

Each test captures the content passed through the tooltip Overlay and verifies it was rendered exactly once, including a real <strong> element rather than escaped generated HTML.

Validation

  • Verified manually in the mynah-ui example app: the previously empty MCP/top-navigation tooltip now displays its text.
  • PR CI runs lint, unit-tests and e2e-linux.
  • Local automated tests were not run; CI is the first execution of the new tests.

Button converted tooltip markdown to HTML before passing it to CardBody, which
then parsed the generated HTML as markdown again. Since raw HTML is escaped by
the centralized parser, icon-only tab bar tooltips rendered without usable text.

Pass the raw label and tooltip markdown to CardBody so it remains the single
owner of rendering. Add coverage for explicit button tooltips and truncated-label
auto-tooltips.
Comment thread src/components/button.ts
tooltipText = '';
}
tooltipText += parseMarkdown(props.tooltip ?? '', { includeLineBreaks: true });
// CardBody owns markdown rendering; pre-rendering here causes its parser to escape the generated HTML.

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.

Instead we should not send tooltip option at all from the LS? keep this tooltip experience as optional

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As optional, do you mean by if LS does not send it, we should not render anything at all instead of an empty bubble?

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.

Yes, LS should decide to show tooltip or not.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually it did check for null description but it did not check for empty string. I added it so it won't render empty string as well

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.

2 participants