Skip class strings containing template syntax (fixes 0.25.0 regression) - #141
Conversation
Since fc7d701 the default finder regex captures class attributes that embed template-language tags (previously the restrictive character class made these attributes non-matches). split_class_tokens then treats whitespace inside the embedded code as a class boundary, so sorting moves tokens across quote and tag boundaries — corrupting ERB ternaries into invalid Ruby or silently changing which classes a branch applies (avencera#140), and reformatting Ruby string interpolation (avencera#124). Leave a matched class string untouched when it contains a template opening delimiter (<%, <?, {{, {%, #{). This restores the 0.24.x behavior of ignoring such attributes while keeping the improved handling of arbitrary values with whitespace. Fixes avencera#140
Note on sort_classes that it expects plain class names (the template-syntax guard lives in sort_file_contents), and add a TODO for tokenizing template tags as opaque units so surrounding static classes can still be sorted.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change adds template-syntax detection for extracted class attribute strings and skips sorting when delimiters are present. It also adds unit coverage for the detection helper and for class strings containing ERB, Ruby interpolation, mustache-style braces, and JavaScript template literals. ChangesTemplate Syntax Guard
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|
| Filename | Overview |
|---|---|
| rustywind-core/src/app.rs | Adds template-syntax skipping to the shared file-sorting path, but the delimiter list can miss suffix fragments after quote-split template matches. |
Reviews (1): Last reviewed commit: "Document template-syntax guard scope" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rustywind-core/src/app.rs (1)
295-308: 🎯 Functional Correctness | 🔵 TrivialConsider whether
${...}template-literal interpolation needs the same guard.The delimiter list covers ERB/EJS, PHP, Handlebars/Jinja/Liquid, and Ruby interpolation, but not JS/TS template-literal interpolation (
${...}), which is a common pattern for conditional classes in JSX/Vue/Svelte template strings (e.g.class={`base ${cond ? 'a' : 'b'}`}). This could hit the same whitespace-splitting corruption this PR fixes for other engines. Given the existing TODO acknowledges more nuanced handling is future work, this may be intentionally out of scope for now — worth confirming if it should be added toTEMPLATE_DELIMITERSin this pass or deferred.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rustywind-core/src/app.rs` around lines 295 - 308, The template-syntax guard in contains_template_syntax is missing JS/TS template-literal interpolation, so add "${" to TEMPLATE_DELIMITERS if this pass should protect JSX/Vue/Svelte class strings like class={\`base ${cond ? 'a' : 'b'}\`}; otherwise explicitly defer it and document that the current guard in app.rs is intentionally limited. Keep the change scoped to the TEMPLATE_DELIMITERS constant and the contains_template_syntax check so the same opaque-syntax protection applies consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@rustywind-core/src/app.rs`:
- Around line 295-308: The template-syntax guard in contains_template_syntax is
missing JS/TS template-literal interpolation, so add "${" to TEMPLATE_DELIMITERS
if this pass should protect JSX/Vue/Svelte class strings like class={\`base
${cond ? 'a' : 'b'}\`}; otherwise explicitly defer it and document that the
current guard in app.rs is intentionally limited. Keep the change scoped to the
TEMPLATE_DELIMITERS constant and the contains_template_syntax check so the same
opaque-syntax protection applies consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e18d97df-e52e-4950-8a6b-d2e5df5bf414
📒 Files selected for processing (1)
rustywind-core/src/app.rs
Add closing delimiters (%>, ?>, }}, %}) so a regex match split by a
quote inside template code is still skipped when the fragment only
contains the tail of a tag, and add ${ for JS template-literal
interpolation (e.g. Lit-style class="p-4 ${cond ? 'a b' : 'c'}"),
which the default regex captures and would corrupt the same way as
ERB.
|
@coderabbitai Added |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
git log --oneline -5
git show b3acfaf -- rustywind-core/src/app.rs || git log --all --oneline | grep -i b3acfafLength of output: 4331
The One thing worth double-checking: with This looks like a solid, well-tested fix. Nice work closing both gaps in the same pass. (°◡°)ノ ✏️ Learnings added
|
|
Thanks @brianfoshee I'll get this reviewed and release 0.25.1 ASAP |
|
@brianfoshee fix is in |
|
Thank you @praveenperera |
2 participants
I filed #140 after noticing my CI fail in a Ruby on Rails project. Here's my attempt at a fix (I worked with Claude Code, for full transparency).
Fixes #140. Also fixes the corruption reported in #124.
Fix
In
sort_file_contents, leave a matched class string untouched when it contains a template opening delimiter:<%(ERB/EJS),<?(PHP),{{/{%(Handlebars/Jinja/Liquid),#{(Ruby string interpolation).This matches the 0.24.x effect of leaving these attributes untouched — 0.24.x's restrictive regex simply didn't match them; this guard produces the same result via a skip after matching — while keeping fc7d701's improvement: arbitrary values with whitespace like
max-w-[min(100%, 500px)]still sort correctly.Because the guard is in the shared
sort_file_contentspath, it also applies to--custom-regex, which fixes the#{ ... ? ... : ... }reformatting reported in #124.Trade-offs
TEMPLATE_DELIMITERS) to keep this PR a targeted regression fix.content-['<?']— is now skipped too.sort_file_contents; callingsort_classesdirectly with a template-bearing string still splits it on whitespace. Its doc comment now states that it expects plain class names.Testing
Added five
test_cases torustywind-core/src/app.rscovering the three repros from #140, the interpolation case from #124, and a mustache-style case.All fail on
mainwith exactly the corruption from the issue and pass with this change.Each asserts
sort_file_contentsis byte-identical on its input, which is what makes--check-formattedpass again — closing the "CI accepts the corrupted output" symptom from #140.Summary by CodeRabbit
class/classNamestrings and leave those segments unchanged instead of reordering tokens.