test: pin down percent escaping in the formatted CSS constants - #81
Merged
Conversation
Every CSS constant is rendered through `String(format:)`, which consumes a bare `%`. `imageCSS` was missing the `%%` escapes, so `max-width: 100%` was emitted as `max-width: 100` - an invalid length that WebKit discards, leaving images unconstrained by the web view width. Add two regression tests: one asserting the generated CSS still carries its percent signs, and one scanning the format strings themselves so a future edit that forgets to double a `%` fails at test time rather than in a layout bug. Requires #76.
There was a problem hiding this comment.
🟡 Changes recommended
The new “input check” doesn’t include all formatted CSS constants (e.g., linkCSS), which weakens the stated goal of preventing silent reintroduction of unescaped percent signs.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR adds regression tests to prevent CSS percent signs (%) from being accidentally stripped when CSS is generated via String(format:), which would silently turn values like 100% into invalid CSS (100) and break layout (notably image sizing).
Changes:
- Adds an output regression test asserting that generated CSS contains key percent-based declarations (and not their stripped forms).
- Adds an input regression test that scans selected
RichTextConstantsformat strings to ensure any%is either escaped (%%) or an intended format specifier (%@,%d).
File summaries
| File | Description |
|---|---|
| Tests/RichTextTests/RichTextSwiftTestingTests.swift | Adds regression tests to verify percent signs survive String(format:) CSS generation and to validate escaping in format-string CSS constants. |
Review details
Suppressed comments (1)
Tests/RichTextTests/RichTextSwiftTestingTests.swift:105
percentBasedConstantsAreEscapedonly checks three formatted CSS constants, butlinkCSSis also passed throughString(format:)(so a future unescaped%there would still regress silently). Include it in the scanned list so the guard applies to all formatted CSS constants.
let formattedConstants = [
RichTextConstants.imageCSS,
RichTextConstants.textCSS,
RichTextConstants.iframeCSS
]
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+84
to
+85
| // Every CSS constant runs through `String(format:)`, which consumes a bare `%`. | ||
| // A literal percent sign therefore has to be written as `%%` in the constant. |
Addresses Copilot review feedback on #81. The input check only walked `imageCSS`, `textCSS` and `iframeCSS`, which left `linkCSS`, `cssTemplate`, `mediaCSSTemplate` and `htmlTemplate` unguarded even though they all go through `String(format:)` too. Turn the check into a parameterised test over the full set, and name the offending constant in the failure message. Also corrects the comment: not every CSS constant is formatted, only the ones carrying substitutions, and the previous wording would have misled the next person into escaping the wrong things.
This was referenced Aug 29, 2026
There was a problem hiding this comment.
🟡 Changes recommended
The new format-specifier whitelist codifies %d for integer formatting even though the code passes a Swift Int, which is a type mismatch on 64-bit and blocks fixing it (e.g., to %ld or by casting).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
Comment on lines
+123
to
+127
| let next = index + 1 < characters.count ? characters[index + 1] : nil | ||
| #expect( | ||
| next == "%" || next == "@" || next == "d", | ||
| "Unescaped percent sign in \(name): a literal % must be written as %%" | ||
| ) |
Addresses the second round of Copilot review feedback on #81. The scanner required every `%` to be followed by `%`, `@` or `d`. That pins the conversion vocabulary down as a side effect: `iframeCSS` passes a Swift `Int` to `%d`, which reads 32 bits of a 64-bit argument, and widening it to `%ld` would have been rejected by this test even though it is the more correct spelling. Test the failure mode instead. A literal percent sign in CSS is always followed by a delimiter (`100%;`, `100% }`, `100%,`), while a conversion is followed by a specifier or a length modifier, so flagging the delimiters catches a missing `%%` without constraining which conversions are allowed. Verified against all seven formatted constants: they pass, the pre-#76 `imageCSS` still reports its three unescaped percent signs, and `%d`, `%ld`, `%s` and `%@` are all accepted.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Problem
Every CSS constant in
RichTextConstantsis rendered throughString(format:), which consumes a bare%. A literal percent sign therefore has to be written as%%in the constant.imageCSSwas missing those escapes, so this constant:was emitted as:
100is not a valid CSS length, so WebKit discarded those declarations. The practical damage was thatmax-width: 100%never applied and images were not constrained to the web view width.#76 fixes the constant. This PR makes the same mistake impossible to reintroduce silently.
Changes
Two regression tests:
max-width: 100%,max-height: 100%, the iframewidth:100%,line-height: 150.0%) and never the stripped form.%is followed by%,@ord. A future edit that forgets to double a percent sign now fails at test time instead of turning into a layout bug that nobody notices for a year.Merge order
This depends on #76 and will fail CI until that is merged.
Refs #76