[fix] Implement markdown escaping for labels in StreamlitMarkdown com…#12695
[fix] Implement markdown escaping for labels in StreamlitMarkdown com…#12695sea-turt1e wants to merge 14 commits intostreamlit:developfrom
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| ({ input, expected }) => { | ||
| render(<StreamlitMarkdown source={input} allowHTML={false} isLabel />) | ||
| const markdownText = screen.getByText(expected) | ||
| expect(markdownText).toBeInTheDocument() |
There was a problem hiding this comment.
According to the TypeScript Test Guide, using queries that throw when elements are not found (like getByText) combined with toBeInTheDocument() assertions is redundant and should be avoided. Since getByText already throws if the element is not found, the toBeInTheDocument() assertion is unnecessary. Either replace toBeInTheDocument() with toBeVisible() or remove the assertion entirely.
| expect(markdownText).toBeInTheDocument() | |
| expect(markdownText).toBeVisible() |
Spotted by Diamond (based on custom rule: Typescript Unit Tests)
Is this helpful? React 👍 or 👎 to let us know.
sfc-gh-lwilby
added
security-assessment-completed
change:feature
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
frontend/lib/src/components/shared/StreamlitMarkdown/StreamlitMarkdown.tsx
Fixed
Show fixed
Hide fixed
sfc-gh-lwilby
added
the
status:needs-product-approval
There was a problem hiding this comment.
Pull Request Overview
This PR implements markdown escaping for widget labels in the StreamlitMarkdown component to prevent single-character tokens (like "+", "-", ">") and list markers from being interpreted as markdown syntax. This ensures that buttons and other widgets display their intended text literally instead of being rendered as markdown elements.
- Added escaping functions to neutralize markdown syntax in label text while preserving whitespace
- Updated the markdown processing logic to conditionally apply escaping when
isLabelis true - Enhanced test coverage to verify literal rendering of markdown tokens in labels
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| StreamlitMarkdown.tsx | Implements markdown escaping logic for labels and integrates it into the markdown processing pipeline |
| StreamlitMarkdown.test.tsx | Adds test cases to verify literal rendering of markdown tokens and fixes assertion method |
| // If the core is empty after trimming, return the original line (it may be all whitespace). | ||
| const trimmedCore = core.trim() | ||
| if (trimmedCore.length === 0) { | ||
| return line | ||
| } |
There was a problem hiding this comment.
The comment on line 675 is misleading. The check core.length === 0 on line 672 already handles empty core content, so this check for trimmedCore.length === 0 handles cases where core contains only whitespace, not empty core. The comment should be updated to reflect this distinction.
| if (SINGLE_CHARACTER_LABEL_TOKENS.has(trimmedCore)) { | ||
| return `${leadingWhitespace}\\${trimmedCore}${trailingWhitespace}` | ||
| } | ||
| // Match unordered list tokens (-, *, +) with optional trailing dot or parenthesis |
There was a problem hiding this comment.
The comment on line 684 incorrectly describes the regex pattern. The regex matches ordered list tokens (numbers followed by dot or parenthesis), not unordered list tokens. The comment should read 'Match ordered list tokens (1., 2), etc.)' instead.
| // Match unordered list tokens (-, *, +) with optional trailing dot or parenthesis | |
| // Match ordered list tokens (1., 2), etc.) |
|
@sea-turt1e I think it would be good to add an e2e test as well. |
…e/streamlit into fix/issue_7359_st_button_empty
…e/streamlit into fix/issue_7359_st_button_empty
|
@sfc-gh-lwilby |
jrieke
added
status:product-approved
|
Looks good from product side! @sfc-gh-lwilby do you want to finish the review for this? |
SummaryThis PR fixes issue #7359 where buttons with labels containing markdown syntax tokens like Changes:
Code QualityThe implementation is well-structured and follows the codebase patterns: Positives:
Minor observation:
Test CoverageFrontend Unit Tests (
E2E Tests (
Note: The Backwards CompatibilityNo breaking changes. This fix:
Security & RiskNo security concerns identified:
Risk Assessment: Low
RecommendationsThe PR is well-implemented. A few optional improvements for consideration:
These are minor suggestions and not blockers for merging. VerdictAPPROVED: This is a clean, well-tested fix for a legitimate bug affecting button labels with markdown tokens. The implementation is sound, test coverage is adequate, and there are no backwards compatibility or security concerns. This is an automated AI review. Please verify the feedback and use your judgment. |
|
Hi @sea-turt1e! I ran a readiness check on this PR to see what needs attention before we can move forward with review. Here's what I found: Status: Not Ready for Review YetGood news: Your PR is well-written with comprehensive tests and already has product approval! However, there are a couple of CI failures that need to be fixed first. What Needs to Be Fixed1. Frontend Linter Failure
|
sfc-gh-lwilby
added
the
not-ready-for-maintainer-review
## Describe your changes This PR fixes issue #7359 where widget labels containing markdown syntax characters (-, +, *, #, >, 1.) would render as empty labels because the markdown parser was interpreting them as list markers, headings, or blockquotes which are then stripped for labels. The fix escapes these markdown syntax patterns when `isLabel` is true, converting them to literal text by adding backslash escapes before markdown is processed. The escaping only applies to patterns followed by whitespace or end of line (e.g., "- item" is escaped but "not-a-list" is not). ## GitHub Issue Link (if applicable) Fixes #7359 ## Testing Plan - Unit Tests: 175 passing tests covering escaped patterns, non-escaped patterns, edge cases with pre-escaped text, and emphasis markdown - E2E Tests: Added test cases for "+" and "1. Something" labels to verify they display correctly in buttons - No additional manual testing needed beyond existing test coverage --- Co-authored-by: sea-turt1e <[email protected]> - See original PR #12695 --- **Contribution License Agreement** By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license. --------- Co-authored-by: Copilot <[email protected]>
|
Closing this since it was fixed in this PR: #13887 |
Describe your changes
GitHub Issue Link (if applicable)
#7359
Testing Plan
yarn workspace @streamlit/lib test src/components/shared/StreamlitMarkdown/StreamlitMarkdown.test.tsxContribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.