chore: add RTL CSS Checker to autofix#1077
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
📝 WalkthroughWalkthroughAdds an RTL consistency check and threads a checker through RTL rule generation: a new script Possibly related PRs
🚥 Pre-merge checks | ❌ 1❌ Failed checks (1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
uno-preset-rtl.ts (1)
52-61:match.replaceis fragile — but safe in thep/mrules only by coincidence.In
directionSizeRTL,match.replace(direction, replacement)replaces the first occurrence of the single character. Forpr-4the firstris the direction, so this works. However, the same pattern used in theroundedandborderhandlers below (lines 161, 181) is broken — see comments there.Consider extracting the "compute suggestion + emit warning" block into a shared helper to reduce the six-fold duplication across handlers and to fix the replacement logic in one place.
♻️ Proposed helper to centralise warning emission and fix replacements
+function emitRtlWarning( + match: string, + direction: string, + replacement: string, + context: RuleContext<any>, + checker?: CollectorChecker, +) { + const fullClass = context.rawSelector || match + const suggestedBase = match.replace(`-${direction}`, `-${replacement}`) + const suggestedClass = fullClass.replace(match, suggestedBase) + const message = `avoid using '${fullClass}', use '${suggestedClass}' instead.` + if (checker) { + checker(message, fullClass) + } else { + warnOnce(`[RTL] ${message}`, fullClass) + } +}Then each handler call site becomes a single line, e.g.:
emitRtlWarning(match, direction!, replacement, context, checker)scripts/rtl-checker.ts (2)
12-46: Mutable closure variables (idx,line) are safe here, but fragile.The checker callback (lines 25-35) captures
idxandlineby reference. This works correctly becauseuno.generate(line)isawaited and the preset rules invoke the checker synchronously within that call. However, if UnoCSS ever deferred the callback (e.g. microtask), the captured values would be stale.A slightly more robust approach is to scope these per iteration:
♻️ Optional: scope variables per iteration
const lines = file.split('\n') for (let i = 0; i < lines.length; i++) { - idx = i + 1 - line = lines[i] - await uno.generate(line) + const currentIdx = i + 1 + const currentLine = lines[i]! + idx = currentIdx + line = currentLine + await uno.generate(currentLine) }Better still, move the
idx/lineinto a small object passed to the checker, but this would require a signature change inCollectorChecker.
22-37: A newcreateGeneratoris instantiated for every.vuefile.Each call to
checkFilecreates a fresh UnoCSS generator (including preset initialisation). For repositories with many Vue files this could be noticeably slow. Consider creating the generator once and passing it in, resetting only thewarningsmap between files.
| } | ||
| } | ||
|
|
||
| check() |
There was a problem hiding this comment.
Unhandled promise rejection — add a .catch() handler.
check() returns Promise<void>. If any file read, glob, or generation throws, the rejection is unhandled. Node will exit with a non-zero code on unhandled rejections, but the error message will be a raw stack trace rather than a clear diagnostic.
🛡️ Proposed fix
-check()
+check().catch((error) => {
+ console.error(`${COLORS.red}RTL check failed: ${error}${COLORS.reset}`)
+ process.exit(1)
+})Based on learnings: "Use error handling patterns consistently".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| check() | |
| check().catch((error) => { | |
| console.error(`${COLORS.red}RTL check failed: ${error}${COLORS.reset}`) | |
| process.exit(1) | |
| }) |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
uno-preset-rtl.ts (1)
46-63: Extract a helper to eliminate the repeated checker/warnOnce pattern.The checker-vs-
warnOncedispatch block is duplicated six times across the file with near-identical structure. Extracting a small helper would reduce duplication and guarantee consistent message formatting (currently the checker message at line 117 uses'. Use 'while the other five use', use ').♻️ Proposed helper and usage
Add a helper near the top of the file:
function emitWarning( fullClass: string, suggestedClass: string, checker?: CollectorChecker, ) { const message = `avoid using '${fullClass}', use '${suggestedClass}' instead.` if (checker) { checker(message, fullClass) } else { warnOnce(`[RTL] Avoid using '${fullClass}'. Use '${suggestedClass}' instead.`, fullClass) } }Then replace every inline block, e.g.:
- if (checker) { - checker(`avoid using '${fullClass}', use '${suggestedClass}' instead.`, fullClass) - } else { - warnOnce(`[RTL] Avoid using '${fullClass}'. Use '${suggestedClass}' instead.`, fullClass) - } + emitWarning(fullClass, suggestedClass, checker)
|
fixed |
|
PR ready to review. |
# Conflicts: # package.json
Co-authored-by: Daniel Roe <daniel@roe.dev>
2 participants
The file paths with errors using absolute path, clicking using relative paths in WebStorm terminal doesn't work (VSCode click works with absolute and relative paths).
This PR includes a fix at
DownloadAnalitics.vuefor atext-rightstyle,preset-wind4adding some rules with higher priority and the RTL preset dind't detect those cases, solved using shorcuts and a hack.This afternoon I'll try to add the fix: https://streamable.com/fr498h