Fix html escaping#30
Conversation
Signed-off-by: Jose Castillo Lema <josecastillolema@gmail.com>
Summary of ChangesHello @josecastillolema, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical issue where special characters in meeting titles, when rendered by Waybar, could lead to markup parsing errors and prevent the calendar from displaying. The core change involves a robust refactor of the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses an HTML escaping issue in meeting titles that was causing errors in Waybar. The core of the fix is a rewritten ellipsis function that is now aware of HTML entities during string truncation, which is a solid improvement. My review includes a suggestion to enhance the robustness of this new function by handling edge cases with small length values. Additionally, I've provided feedback on the new tests, recommending more precise assertions to ensure the function's behavior is validated as accurately as possible. Overall, these changes are a valuable fix for the reported problem.
| # Truncate based on display length, keeping entities intact | ||
| result = [] | ||
| current_display_len = 0 | ||
| target_len = length - 3 # Leave room for "..." | ||
| i = 0 | ||
|
|
||
| while i < len(clean_string) and current_display_len < target_len: | ||
| if clean_string[i] == "&": | ||
| end = clean_string.find(";", i) | ||
| if end != -1: | ||
| result.append(clean_string[i : end + 1]) | ||
| i = end + 1 | ||
| current_display_len += 1 | ||
| continue | ||
| result.append(clean_string[i]) | ||
| current_display_len += 1 | ||
| i += 1 | ||
|
|
||
| return "".join(result) + "..." |
There was a problem hiding this comment.
The current implementation of ellipsis doesn't handle small length values correctly. If length is less than 4, target_len can become zero or negative, causing the while loop to not execute. This results in the function returning just "...", which is 3 characters long and can be longer than the requested length. The function should be robust enough to handle any length value gracefully, for example by truncating without adding an ellipsis for small lengths.
# Truncate based on display length, keeping entities intact
result = []
current_display_len = 0
if length <= 3:
target_len = length
suffix = ""
else:
target_len = length - 3 # Leave room for "..."
suffix = "..."
i = 0
while i < len(clean_string) and current_display_len < target_len:
if clean_string[i] == "&":
end = clean_string.find(";", i)
if end != -1:
result.append(clean_string[i : end + 1])
i = end + 1
current_display_len += 1
continue
result.append(clean_string[i])
current_display_len += 1
i += 1
return "".join(result) + suffix| def test_ellipsis_html_entity_not_broken(): | ||
| # & should count as 1 display char and not be split | ||
| result = ellipsis("Perf&Scale meeting", 15) | ||
| assert "&" in result or result.endswith("...") | ||
| # Ensure no broken entity like & without semicolon | ||
| assert "&" not in result or "&" in result | ||
|
|
||
|
|
||
| def test_ellipsis_html_entity_counts_as_one_char(): | ||
| # "Test&Go" has display length 9 (Test&Go + 2 chars for entity displayed as 1) | ||
| # With length=12, should not truncate | ||
| result = ellipsis("Test&Go", 12) | ||
| assert result == "Test&Go" | ||
|
|
||
|
|
||
| def test_ellipsis_numeric_entity_not_broken(): | ||
| # ' is apostrophe, should count as 1 display char | ||
| result = ellipsis("What's New in Tech", 15) | ||
| # Should not have broken entity | ||
| assert "'" not in result or "'" in result | ||
|
|
||
|
|
||
| def test_ellipsis_strips_html_tags(): | ||
| result = ellipsis("<span>Hello</span> World", 15) | ||
| assert "<span>" not in result | ||
| assert "Hello World" in result or "Hello Wor..." in result |
There was a problem hiding this comment.
The assertions in these new tests for ellipsis could be more precise to make them more robust.
For example, in test_ellipsis_html_entity_not_broken, assert "&" in result or result.endswith("...") is weak because it would pass for incorrect results like ... or Perf.... A precise check like assert result == "Perf&Scale m..." would be better.
Similarly, in test_ellipsis_strips_html_tags, for length=15 the result is exactly "Hello World", so assert result == "Hello World" would be more accurate. If testing truncation (e.g., with length=10), the assertion should check for the exact truncated string, like assert result == "Hello W...".
Precise assertions ensure the function behaves exactly as expected.
|
very nice thanks! |
2 participants
When meetings include some specific special chars, i.e.:
&,', etc. I am observing some waybar errors and calendar does not appear:This PR fixes this error and adds some tests.