feat: SVG text truncation and overflow protection#81
Conversation
…nderer Prevents long text fields (game name, developer, player name, context entries, client URL, etc.) from overflowing their bounding boxes in the generated SVG card. Adds `truncate_with_ellipsis` helper applied before URI-encoding, and SVG clip-path regions for safety. Includes unit tests and an overflow stress test. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary of ChangesHello, 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 introduces robust text handling mechanisms for SVG generation to prevent text overflow. It implements a new utility function for truncating strings with an ellipsis and applies this truncation across various text fields in the SVG card. Additionally, it enhances visual integrity by adding SVG clip paths to critical areas, ensuring that long text content remains within its designated boundaries. 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. 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. Footnotes
|
|
Claude finished @starknetdev's task —— View job Claude Review - General Engineering Reviewrun=23447879477 attempt=1 sha=5f89986dc734b27b9fdcc60ef03522b0e38dfd0a scope=general Todo List
Review AnalysisI executed the specified git diff command to check for changes outside the git diff f81e2ff983e4c0681541e09a587b5c4430a7857b...5f89986dc734b27b9fdcc60ef03522b0e38dfd0a -- . ':(exclude)packages/**'Result: No output - confirming there are no changes outside the packages directory. The only file modified in this PR is FindingsNo issues found. Summary0 CRITICAL, 0 HIGH, 0 MEDIUM, 0 LOW, 0 INFO |
📝 WalkthroughWalkthroughIntroduces Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review - General Engineering ReviewReview process failed to complete. |
|
Claude finished @starknetdev's task —— View job Claude Review - Cairo/Starknet Contract Reviewrun=23447879477 attempt=1 sha=5f89986dc734b27b9fdcc60ef03522b0e38dfd0a scope=packages Todo List
No issues found. The PR introduces a Highlights:
Test Coverage Strengths:
The clip paths and truncation limits are sensibly chosen based on the SVG layout dimensions. The implementation correctly applies truncation before URI encoding to ensure limits reflect visible characters. Summary: 0 CRITICAL, 0 HIGH, 0 MEDIUM, 0 LOW, 0 INFO |
Codex Review - Cairo/Starknet Contract ReviewReview process failed to complete. |
![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 introduces a truncate_with_ellipsis function to prevent text overflow in generated SVG elements. This function is applied to various text fields within the create_default_svg function, and new clipPath elements are added to visually clip the text. The changes also include new unit tests for the truncation logic and an SVG overflow stress test. Feedback includes addressing a potential integer underflow in truncate_with_ellipsis, replacing magic numbers with named constants for truncation limits, and strengthening the SVG overflow stress test to verify actual truncation.
| fn truncate_with_ellipsis(text: ByteArray, max_chars: u32) -> ByteArray { | ||
| if text.len() <= max_chars { | ||
| return text; | ||
| } | ||
| let mut result: ByteArray = ""; | ||
| let limit = max_chars - 3; | ||
| let mut i: u32 = 0; | ||
| loop { | ||
| if i >= limit { | ||
| break; | ||
| } | ||
| result.append_byte(text.at(i).unwrap()); | ||
| i += 1; | ||
| } | ||
| result.append(@"..."); | ||
| result | ||
| } |
There was a problem hiding this comment.
The current implementation of truncate_with_ellipsis has a potential integer underflow issue. If max_chars is less than 3, the expression max_chars - 3 will underflow and cause a panic, as max_chars is a u32.
While the current usages in create_default_svg all use values greater than 3, the function is not robust for general use. It's better to handle this edge case to prevent future panics.
I've refactored the function to handle cases where max_chars <= 3 by simply truncating the text without adding an ellipsis. This version is safer and also slightly more readable by using a while loop.
fn truncate_with_ellipsis(text: ByteArray, max_chars: u32) -> ByteArray {
if text.len() <= max_chars {
return text;
}
let mut result: ByteArray = "";
let limit = if max_chars > 3 { max_chars - 3 } else { max_chars };
let mut i: u32 = 0;
while i < limit {
result.append_byte(text.at(i).unwrap());
i += 1;
}
if max_chars > 3 {
result.append(@"...");
}
result
}
| let _game_name = uri_encode(truncate_with_ellipsis(format!("{}", game_metadata.name), 17)); | ||
| let _developer = uri_encode(truncate_with_ellipsis(format!("{}", game_metadata.developer), 21)); | ||
| let _genre = uri_encode(truncate_with_ellipsis(format!("{}", game_metadata.genre), 29)); |
There was a problem hiding this comment.
The character limits for truncation (17, 21, 29, etc.) are used as magic numbers. This makes the code harder to read and maintain, as the purpose of these specific numbers isn't immediately clear. It's also prone to errors if values need to be updated, as they are scattered throughout the function.
To improve readability and maintainability, please define these limits as constants with descriptive names at the top of the create_default_svg function or in a dedicated constants block. For example:
const GAME_NAME_MAX_CHARS: u32 = 17;
const DEVELOPER_MAX_CHARS: u32 = 21;
const GENRE_MAX_CHARS: u32 = 29;
// ... then use them
let _game_name = uri_encode(truncate_with_ellipsis(format!("{}", game_metadata.name), GAME_NAME_MAX_CHARS));
let _developer = uri_encode(truncate_with_ellipsis(format!("{}", game_metadata.developer), DEVELOPER_MAX_CHARS));
let _genre = uri_encode(truncate_with_ellipsis(format!("{}", game_metadata.genre), GENRE_MAX_CHARS));This should be applied to all magic numbers used with truncate_with_ellipsis in this function.
|
|
||
| stop_cheat_block_timestamp_global(); | ||
|
|
||
| assert!(svg_result.len() > 0, "SVG should be generated with overflow inputs"); |
There was a problem hiding this comment.
The assertion assert!(svg_result.len() > 0, ...) in test_svg_overflow_stress is quite weak. It only confirms that an SVG string was generated, but doesn't verify that truncation is actually working as expected.
To make this test more meaningful, you could add assertions to check that the generated SVG string contains the ellipsis (...), which would indicate that truncation has occurred. Since ByteArray doesn't have a built-in contains method, you might need a small helper for this.
A stronger assertion would provide more confidence that the overflow protection is effective. For example, you could check for the presence of ... which is expected due to multiple fields being truncated.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/utilities/src/renderer/svg.cairo (2)
1100-1197: The new tests don’t lock in the overflow contract yet.The helper tests still miss
max_chars < 3and non-ASCII input, and the stress test’ssvg_result.len() > 0assertion would still pass if the ellipsis logic or clip-path wiring regressed. Please add concrete assertions for expected ellipsized fragments / clip IDs plus one under-limit control case.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/utilities/src/renderer/svg.cairo` around lines 1100 - 1197, Add missing edge-case assertions: extend tests for truncate_with_ellipsis to cover max_chars < 3 (e.g., 0,1,2) and a non-ASCII input (e.g., "éåß…") asserting exact returned strings; in test_svg_overflow_stress replace the loose assert!(svg_result.len() > 0) with explicit checks that create_default_svg produced the expected ellipsized fragments (match substrings produced by truncate_with_ellipsis for name/description), that a generated clip-path/clipId (search for "clipPath" or "clipPathId" string) appears exactly once, and add one under-limit control case (input shorter than limit) asserting no ellipsis is present. Use the functions/traces truncate_with_ellipsis, create_default_svg and the svg_result variable to locate where to add these assertions.
148-166: Function is byte-based; safe for ASCII inputs only.
truncate_with_ellipsisusesByteArray.at(i)which indexes bytes rather than UTF-8 code points. Since all current inputs (game names, URLs, felt252 conversions) are ASCII, each character equals one byte and the function works correctly. However, if non-ASCII input (emoji, multibyte characters) is added in the future, this implementation could split a multibyte sequence. Additionally, line 155 subtracts 3 without guarding againstmax_chars < 3, though all current callers safely use values ≥ 12.Consider documenting the ASCII assumption or refactoring to explicitly handle UTF-8 boundaries and validate
max_chars >= 3for future robustness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/utilities/src/renderer/svg.cairo` around lines 148 - 166, truncate_with_ellipsis currently slices ByteArray by bytes using ByteArray.at(i) which can split multibyte UTF‑8 characters and also unguardedly computes limit = max_chars - 3; update the function truncate_with_ellipsis to either (A) operate on UTF‑8 code points (iterate characters/char boundaries) so you never break multibyte sequences, or (B) explicitly document the ASCII-only assumption and add a guard that validates max_chars >= 3 and returns/input-error if not; ensure you reference and change the byte-based loop (ByteArray.at and result.append_byte) to a safe character-aware iteration or add an early validation for max_chars and a clear comment next to truncate_with_ellipsis about ASCII-only guarantees.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/utilities/src/renderer/svg.cairo`:
- Around line 1100-1197: Add missing edge-case assertions: extend tests for
truncate_with_ellipsis to cover max_chars < 3 (e.g., 0,1,2) and a non-ASCII
input (e.g., "éåß…") asserting exact returned strings; in
test_svg_overflow_stress replace the loose assert!(svg_result.len() > 0) with
explicit checks that create_default_svg produced the expected ellipsized
fragments (match substrings produced by truncate_with_ellipsis for
name/description), that a generated clip-path/clipId (search for "clipPath" or
"clipPathId" string) appears exactly once, and add one under-limit control case
(input shorter than limit) asserting no ellipsis is present. Use the
functions/traces truncate_with_ellipsis, create_default_svg and the svg_result
variable to locate where to add these assertions.
- Around line 148-166: truncate_with_ellipsis currently slices ByteArray by
bytes using ByteArray.at(i) which can split multibyte UTF‑8 characters and also
unguardedly computes limit = max_chars - 3; update the function
truncate_with_ellipsis to either (A) operate on UTF‑8 code points (iterate
characters/char boundaries) so you never break multibyte sequences, or (B)
explicitly document the ASCII-only assumption and add a guard that validates
max_chars >= 3 and returns/input-error if not; ensure you reference and change
the byte-based loop (ByteArray.at and result.append_byte) to a safe
character-aware iteration or add an early validation for max_chars and a clear
comment next to truncate_with_ellipsis about ASCII-only guarantees.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6117226d-68c0-4755-bd43-35a3d7f9d851
📒 Files selected for processing (1)
packages/utilities/src/renderer/svg.cairo
Fixes CI warning: "Unused import: game_components_utilities::renderer::svg::Into" Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Claude finished @starknetdev's task —— View job Claude Review - General Engineering Reviewrun=23448921310 attempt=1 sha=fdcd0f42692a30ad3f3738419f55ddbd3921176b scope=general Todo List
Review AnalysisI executed the specified git diff command to check for changes outside the git diff f81e2ff983e4c0681541e09a587b5c4430a7857b...fdcd0f42692a30ad3f3738419f55ddbd3921176b -- . ':(exclude)packages/**'Result: No output - confirming there are no changes outside the packages directory. The only file modified in this PR is FindingsNo issues found. Summary0 CRITICAL, 0 HIGH, 0 MEDIUM, 0 LOW, 0 INFO |
Codex Review - General Engineering ReviewReview process failed to complete. |
|
Claude finished @starknetdev's task —— View job Claude Review - Cairo/Starknet Contract Reviewrun=23448921310 attempt=1 sha=fdcd0f42692a30ad3f3738419f55ddbd3921176b scope=packages Todo List
No issues found. The PR introduces a Highlights:
Test Coverage Strengths:
The clip paths and truncation limits are sensibly chosen based on the SVG layout dimensions. The implementation correctly applies truncation before URI encoding to ensure limits reflect visible characters. Summary: 0 CRITICAL, 0 HIGH, 0 MEDIUM, 0 LOW, 0 INFO |
Codex Review - Cairo/Starknet Contract ReviewReview process failed to complete. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/utilities/src/renderer/svg.cairo (1)
1100-1127: Good test coverage for truncation helper.The tests cover the key scenarios well. Consider adding a test for the edge case when
max_chars = 3(minimum value that produces "..." without risk of issues):📝 Optional: Add edge case test
#[test] fn test_truncate_with_ellipsis_min_limit() { let result = truncate_with_ellipsis("ABCDE", 3); // With max_chars=3 and limit=0, result should be "..." assert!(result == "...", "Min limit should produce just ellipsis"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/utilities/src/renderer/svg.cairo` around lines 1100 - 1127, Add a unit test for the edge case where max_chars == 3 to ensure truncate_with_ellipsis returns only the ellipsis; specifically, create a test (e.g., test_truncate_with_ellipsis_min_limit) that calls truncate_with_ellipsis("ABCDE", 3) and asserts the result == "..." so the helper's behavior at the minimum truncation boundary is validated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/utilities/src/renderer/svg.cairo`:
- Around line 147-165: In truncate_with_ellipsis, guard against underflow when
max_chars < 3 by computing the prefix length with a safe subtraction (e.g., use
a saturating or conditional subtraction: let limit = if max_chars > 3 {
max_chars - 3 } else { 0 }) and then handle the small-max case by returning an
appropriate ellipsis-truncated result (e.g., return the first max_chars bytes of
"..." when max_chars <= 3); update the logic in truncate_with_ellipsis to use
this safe limit computation and to avoid iterating when limit == 0.
---
Nitpick comments:
In `@packages/utilities/src/renderer/svg.cairo`:
- Around line 1100-1127: Add a unit test for the edge case where max_chars == 3
to ensure truncate_with_ellipsis returns only the ellipsis; specifically, create
a test (e.g., test_truncate_with_ellipsis_min_limit) that calls
truncate_with_ellipsis("ABCDE", 3) and asserts the result == "..." so the
helper's behavior at the minimum truncation boundary is validated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f9726665-a353-4bf0-9e6f-cdc2bae15450
📒 Files selected for processing (1)
packages/utilities/src/renderer/svg.cairo
| /// Truncate a `ByteArray` to `max_chars` visible characters, appending "..." if truncated. | ||
| /// Apply BEFORE `uri_encode` so limits are based on visible characters, not encoded bytes. | ||
| fn truncate_with_ellipsis(text: ByteArray, max_chars: u32) -> ByteArray { | ||
| if text.len() <= max_chars { | ||
| return text; | ||
| } | ||
| let mut result: ByteArray = ""; | ||
| let limit = max_chars - 3; | ||
| let mut i: u32 = 0; | ||
| loop { | ||
| if i >= limit { | ||
| break; | ||
| } | ||
| result.append_byte(text.at(i).unwrap()); | ||
| i += 1; | ||
| } | ||
| result.append(@"..."); | ||
| result | ||
| } |
There was a problem hiding this comment.
Potential underflow when max_chars < 3.
If max_chars is less than 3 and text.len() > max_chars, the expression max_chars - 3 will underflow (u32 wraps around), causing the loop to iterate an extremely large number of times.
Current call sites use values ≥ 12, so this is safe in practice. However, consider adding a guard for defensive coding:
🛡️ Proposed defensive fix
fn truncate_with_ellipsis(text: ByteArray, max_chars: u32) -> ByteArray {
- if text.len() <= max_chars {
+ if text.len() <= max_chars || max_chars < 4 {
return text;
}Also note: the doc comment mentions "visible characters" but ByteArray.len() counts bytes. For multi-byte UTF-8 characters, truncation could occur mid-character. This is likely acceptable given the ASCII-dominated inputs (game names, player names), but worth documenting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/utilities/src/renderer/svg.cairo` around lines 147 - 165, In
truncate_with_ellipsis, guard against underflow when max_chars < 3 by computing
the prefix length with a safe subtraction (e.g., use a saturating or conditional
subtraction: let limit = if max_chars > 3 { max_chars - 3 } else { 0 }) and then
handle the small-max case by returning an appropriate ellipsis-truncated result
(e.g., return the first max_chars bytes of "..." when max_chars <= 3); update
the logic in truncate_with_ellipsis to use this safe limit computation and to
avoid iterating when limit == 0.
2 participants
Summary
truncate_with_ellipsishelper that caps visible characters and appends "..." before URI-encoding<clipPath>regions as a safety net for player name, left/right panels, and context areaTest plan
snforge test rendererpasses (unit + stress tests)🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests