-
Notifications
You must be signed in to change notification settings - Fork 8
fix: stop truncating single-paragraph tooltips to their first inline element #422
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
JakeSCahill
wants to merge
1
commit into
main
Choose a base branch
from
jake/fix-tooltip-inline-truncation
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
112 changes: 112 additions & 0 deletions
112
tests/property-tooltips/truncate-description-html.test.js
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,112 @@ | ||
| 'use strict' | ||
|
|
||
| // Verifies src/js/19-property-tooltips.js's truncateDescriptionHtml against | ||
| // the REAL implementation (extracted from the shipped source, not a | ||
| // reimplementation), run inside a real browser page so document.createElement | ||
| // behaves exactly as it does on the live site. | ||
| // | ||
| // The bug this guards: render-property-descriptions.js emits a single | ||
| // paragraph's description as bare inline HTML with no <p> wrapper -- by | ||
| // design, "what a tooltip wants". container.children only ever counts | ||
| // elements, never text nodes, so a single paragraph containing several | ||
| // inline elements (a <code> span, a link, another <code> span) has more than | ||
| // one "child" despite being one block of prose. The old implementation read | ||
| // that as multiple paragraphs and truncated to blocks[0], silently dropping | ||
| // every text node around it. Live examples this actually did in production: | ||
| // tombstone_retention_ms's tooltip showed only "cloud_storage_enabled" (the | ||
| // first of three <code> spans inside its one real paragraph), and | ||
| // kafka_max_message_size_upper_limit_bytes's tooltip showed only a bare link | ||
| // reading "max.message.bytes" (the first inline element, a link wrapping a | ||
| // <code>, inside its one real paragraph). | ||
|
|
||
| const test = require('node:test') | ||
| const assert = require('node:assert/strict') | ||
| const path = require('node:path') | ||
| const fs = require('node:fs') | ||
| const puppeteer = require('puppeteer') | ||
|
|
||
| const ROOT = path.join(__dirname, '..', '..') | ||
| const SRC = fs.readFileSync(path.join(ROOT, 'src/js/19-property-tooltips.js'), 'utf8') | ||
|
|
||
| // Extract just the BLOCK_TAGS constant and the function under test out of | ||
| // the IIFE -- the file as a whole assumes fetch/localStorage globals this | ||
| // test never exercises. | ||
| const BLOCK = SRC.slice( | ||
| SRC.indexOf('var BLOCK_TAGS ='), | ||
| SRC.indexOf('function createPropertyTooltip') | ||
| ) | ||
|
|
||
| // Real production description_html, captured from docs.redpanda.com's | ||
| // topic-properties page (verified live, then fixed here). | ||
| const TOMBSTONE_RETENTION_MS = | ||
| 'The retention time for tombstone records in a compacted topic. For Tiered ' + | ||
| 'Storage v1, cannot be enabled at the same time as any of ' + | ||
| '<code>cloud_storage_enabled</code>, <code>cloud_storage_enable_remote_read</code>, ' + | ||
| 'or <code>cloud_storage_enable_remote_write</code>. This restriction does not ' + | ||
| 'apply to topics that use <a href="/streaming/current/manage/tiered-storage/' + | ||
| '#tiered-storage-versions" class="xref page">Tiered Storage v2</a>, available ' + | ||
| 'starting in Redpanda v26.2. A typical default setting is <code>86400000</code>, ' + | ||
| 'or 24 hours.' | ||
|
|
||
| const KAFKA_MAX_MESSAGE_SIZE_UPPER_LIMIT_BYTES = | ||
| 'The maximum value you can set for the <a href="/streaming/current/reference/' + | ||
| 'properties/topic-properties/#max-message-bytes" class="xref page">' + | ||
| '<code>max.message.bytes</code></a> topic property. When set to <code>null</code>, ' + | ||
| 'no limit is enforced.' | ||
|
|
||
| let browser | ||
| let page | ||
|
|
||
| test.before(async () => { | ||
| browser = await puppeteer.launch({ | ||
| headless: true, | ||
| args: ['--no-sandbox', '--disable-setuid-sandbox', '--disable-dev-shm-usage'], | ||
| }) | ||
| page = await browser.newPage() | ||
| await page.evaluate(BLOCK + '\nwindow.__truncateDescriptionHtml = truncateDescriptionHtml;') | ||
| }) | ||
|
|
||
| test.after(async () => { | ||
| await browser.close() | ||
| }) | ||
|
|
||
| async function truncate (html, summaryOnly) { | ||
| return page.evaluate( | ||
| (h, s) => window.__truncateDescriptionHtml(h, s), | ||
| html, | ||
| summaryOnly | ||
| ) | ||
| } | ||
|
|
||
| test('a single paragraph with several inline elements is not mangled', async () => { | ||
| const result = await truncate(TOMBSTONE_RETENTION_MS, true) | ||
| assert.equal(result, TOMBSTONE_RETENTION_MS) | ||
| assert.match(result, /tombstone records/) | ||
| }) | ||
|
|
||
| test('a single paragraph starting with a link is not mangled', async () => { | ||
| const result = await truncate(KAFKA_MAX_MESSAGE_SIZE_UPPER_LIMIT_BYTES, true) | ||
| assert.equal(result, KAFKA_MAX_MESSAGE_SIZE_UPPER_LIMIT_BYTES) | ||
| assert.match(result, /maximum value/) | ||
| }) | ||
|
|
||
| test('real multi-paragraph content still truncates to the first paragraph', async () => { | ||
| const html = '<p>First real paragraph of prose.</p><p>Second paragraph that should not appear.</p>' | ||
| const result = await truncate(html, true) | ||
| assert.equal(result, '<p>First real paragraph of prose.</p><p>…</p>') | ||
| }) | ||
|
|
||
| test('a list still truncates, since a list is real block structure', async () => { | ||
| const html = '<p>Intro paragraph.</p><ul><li>one</li><li>two</li></ul>' | ||
| const result = await truncate(html, true) | ||
| assert.equal(result, '<p>Intro paragraph.</p><p>…</p>') | ||
| }) | ||
|
|
||
| test('summaryOnly=false returns the html untouched regardless of structure', async () => { | ||
| const result = await truncate(TOMBSTONE_RETENTION_MS, false) | ||
| assert.equal(result, TOMBSTONE_RETENTION_MS) | ||
| }) | ||
|
|
||
| test('empty html returns an empty string', async () => { | ||
| assert.equal(await truncate('', true), '') | ||
| }) |
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Count top-level block elements before truncation.
A fragment such as
<p>Summary.</p><code>suffix</code>has one block element. This condition setshasBlockStructureand sees two element children, then drops the inline suffix. Count recognized block elements and truncate only when that count is greater than one.Proposed fix
🤖 Prompt for AI Agents