fix: severity color contrast #1170
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
|
@userquin is attempting to deploy a commit to the serhalp's projects Team on Vercel. A member of the Team first needs to authorize it. |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR updates multiple UI class strings to add dark-mode-aware variants (severity colours, outdated-dependency statuses, badges, tags, deprecated/version styling, icons, and small presentational tweaks), updates a Nuxt packument test fixture with new version/metadata entries, and adds Shiki helpers including Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 Comment |
|
fixing shiki colors with light theme (github-light theme) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server/utils/shiki.ts (2)
110-112:⚠️ Potential issue | 🟠 MajorSanitise the language before interpolating into the class attribute.
languagecan be user-controlled via code fences; inserting it raw into HTML risks attribute injection. Consider restricting it to a safe character set.Proposed fix
- return `<pre><code class="language-${language}">${escaped}</code></pre>\n` + const safeLanguage = language.replace(/[^a-z0-9_-]/gi, '') || 'plaintext' + return `<pre><code class="language-${safeLanguage}">${escaped}</code></pre>\n`
90-109:⚠️ Potential issue | 🟡 MinorRemove language alias guard to enable alias highlighting.
The
loadedLangscheck prevents language aliases (e.g.gjs,gts) from reachingcodeToHtml. In Shiki v3.21.0,getLoadedLanguages()returns canonical language IDs, not aliases, soloadedLangs.includes('gjs')will be false even though thelangAliasmapping andcodeToHtmlsupport it. Remove the guard and rely on the existingtry/catchto handle any unsupported languages—this aligns with Shiki's design.Proposed fix
export function highlightCodeSync(shiki: HighlighterCore, code: string, language: string): string { - const loadedLangs = shiki.getLoadedLanguages() - - if (loadedLangs.includes(language as never)) { - try { - let html = shiki.codeToHtml(code, { - lang: language, - themes: { light: 'github-light', dark: 'github-dark' }, - defaultColor: 'dark', - }) - // Remove inline style from <pre> tag so CSS can control appearance - html = html.replace(/<pre([^>]*) style="[^"]*"/, '<pre$1') - // Shiki doesn't encode > in text content (e.g., arrow functions =>) - // We need to encode them for HTML validation - return escapeRawGt(html) - } catch { - // Fall back to plain - } - } + try { + let html = shiki.codeToHtml(code, { + lang: language, + themes: { light: 'github-light', dark: 'github-dark' }, + defaultColor: 'dark', + }) + // Remove inline style from <pre> tag so CSS can control appearance + html = html.replace(/<pre([^>]*) style="[^"]*"/, '<pre$1') + // Shiki doesn't encode > in text content (e.g., arrow functions =>) + // We need to encode them for HTML validation + return escapeRawGt(html) + } catch { + // Fall back to plain + }Note:
server/utils/code-highlight.tshas the same pattern at line 272–275 and would benefit from the same fix.
| size="small" | ||
| :to="typesHref" | ||
| classicon="i-carbon-checkmark" | ||
| classicon="i-carbon:checkmark" |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for old-style icon syntax (i-carbon-name pattern)
echo "=== Old-style i-carbon-* syntax (hyphens) ==="
rg "i-carbon-[a-z]" --type vue -g '!node_modules' | wc -l
echo "Count of files/lines with old syntax:"
rg "i-carbon-[a-z]" --type vue -g '!node_modules' -l | wc -l
echo ""
echo "=== New-style i-carbon:* syntax (colons) ==="
rg "i-carbon:[a-z]" --type vue -g '!node_modules' | wc -l
echo "Count of files/lines with new syntax:"
rg "i-carbon:[a-z]" --type vue -g '!node_modules' -l | wc -l
echo ""
echo "=== Detailed breakdown of new-style syntax ==="
rg "i-carbon:[a-z]+" --type vue -g '!node_modules' -o | sort | uniq -c | sort -rn | head -20
echo ""
echo "=== Detailed breakdown of old-style syntax ==="
rg "i-carbon-[a-z]+" --type vue -g '!node_modules' -o | sort | uniq -c | sort -rn | head -20Repository: npmx-dev/npmx.dev
Length of output: 517
🏁 Script executed:
#!/bin/bash
# Check for old-style icon syntax (i-carbon-name pattern)
echo "=== Old-style i-carbon-* syntax (hyphens) in .vue files ==="
rg "i-carbon-[a-z]" -g '*.vue' | wc -l
echo ""
echo "=== New-style i-carbon:* syntax (colons) in .vue files ==="
rg "i-carbon:[a-z]" -g '*.vue' | wc -l
echo ""
echo "=== Sample of new-style syntax matches ==="
rg "i-carbon:[a-z]+" -g '*.vue' -o | sort | uniq -c | sort -rn | head -20
echo ""
echo "=== Sample of old-style syntax matches ==="
rg "i-carbon-[a-z]+" -g '*.vue' -o | sort | uniq -c | sort -rn | head -20
echo ""
echo "=== Check the specific file mentioned ==="
if [ -f "app/components/Package/MetricsBadges.vue" ]; then
echo "File exists. Checking line 66:"
sed -n '60,72p' "app/components/Package/MetricsBadges.vue" | cat -n
fiRepository: npmx-dev/npmx.dev
Length of output: 3334
Update icon syntax to use colons consistently across all Vue components.
The codebase currently has mixed icon syntax: 202 instances of the new colon-based style (i-carbon:checkmark) and 71 instances of the old hyphen-based style (i-carbon-warning). This file correctly uses the new syntax, but other components still use hyphens. Refactor all remaining i-carbon-* patterns to i-carbon:* for consistency.
There was a problem hiding this comment.
This is an small optimization for UnoCSS preset-icons: for single icon name (without dash) the result is the same, but for long icon names using : instead - will resolve the collection directly, the pattern is i-<collection>[-:]<icon>, and so for i-carbon:a-b-c-d we skip some combinations.
We need to find a way to fix this, it is hard since we can add more and more collections and ppl should use : instead -.
There was a problem hiding this comment.
@userquin, thank you for the explanation! That's a valuable optimization detail about UnoCSS preset-icons. The direct collection resolution with : for multi-dash icon names makes sense.
Here are several approaches to enforce this pattern consistently:
1. ESLint Custom Rule
Create a custom ESLint rule that detects i-<collection>-<icon> patterns and suggests using : instead. This would catch violations during development.
2. Pre-commit Hook
Add a Git hook that scans staged files for the old pattern:
# .husky/pre-commit or similar
if git diff --cached --name-only | grep -E '\.(vue|ts|js)$' | xargs grep -E 'i-carbon-[a-z]'; then
echo "❌ Found old icon syntax (i-carbon-name). Please use i-carbon:name instead."
exit 1
fi3. Automated Migration Script
I can help create a script to fix all remaining 71 instances automatically. Would you like me to generate a script that:
- Finds all
i-carbon-*patterns - Replaces them with
i-carbon:* - Shows a diff for review before applying?
4. Documentation
Add this to your contributing guidelines or a dedicated icon usage document explaining:
- The pattern:
i-<collection>:<icon-name> - Why colons are preferred (UnoCSS optimization)
- Common collections used in the project
5. CI Check
Add a CI step that fails if old syntax is detected in new code:
- name: Check icon syntax
run: |
if grep -r "i-carbon-[a-z]" app/ --include="*.vue"; then
echo "Old icon syntax detected"
exit 1
fiWould you like me to help implement any of these solutions? I can generate the migration script or create the linting rules for you.
/ᐠ｡ꞈ｡ᐟ
( thinking )
/> 📝 <\
✏️ Learnings added
Learnt from: userquin
Repo: npmx-dev/npmx.dev PR: 1170
File: app/components/Package/MetricsBadges.vue:66-66
Timestamp: 2026-02-08T15:02:02.232Z
Learning: In Vue components, use colon syntax for UnoCSS preset-icons (e.g., `i-carbon:checkmark` instead of `i-carbon-checkmark`). For long icon names with dashes, the colon syntax provides a performance optimization by helping UnoCSS resolve the collection directly without trying different combinations.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: alexdln
Repo: npmx-dev/npmx.dev PR: 838
File: app/pages/package/[...package].vue:445-449
Timestamp: 2026-02-03T13:59:33.392Z
Learning: The copy button pattern in app/pages/package/[...package].vue may be made into a reusable component or pattern in the future, but currently it's acceptable to keep it inline with the CSS-only approach for smooth animations.
Learnt from: skaldebane
Repo: npmx-dev/npmx.dev PR: 760
File: app/components/Compare/FacetCard.vue:134-136
Timestamp: 2026-02-06T07:13:58.289Z
Learning: In `app/components/Compare/FacetCard.vue`, the facet values use `dir="auto"` rather than `dir="ltr"` because the values are not always numbers/sizes and can contain mixed content types.
There was a problem hiding this comment.
you're killing everyone, there are still a lot of icons using i-<collection>-<icon>, and Windows users won't be able to lint on local (like me)
There was a problem hiding this comment.
@userquin, I sincerely apologize! You're absolutely right—I got carried away with enforcement suggestions that would be disruptive and not Windows-friendly.
Let me take a much more practical approach:
Gradual, Optional Migration
Instead of breaking builds or forcing everyone to use bash hooks, here are gentler options:
1. One-time migration script (Node.js/JavaScript)
I can create a cross-platform Node.js script that you can run once to migrate all remaining old-syntax icons. This would be:
- Completely optional
- Works on Windows, Mac, Linux
- Shows a diff before applying changes
- Can be run at your convenience
2. Documentation only
Simply document the preferred syntax in your contributing guide for future reference, without any enforcement. New code naturally adopts it over time.
3. Optional VS Code snippet
Add a VS Code snippet that auto-completes with the colon syntax, making it easier for contributors to use the right pattern without forcing it.
Would you like me to create a Node.js migration script that you can review and run when convenient? It would be a simple, non-intrusive way to update the remaining 71 instances without impacting anyone's workflow.
Again, sorry for the overly aggressive suggestions!
/ᐠ - ˕ -マ
( sorry! )
/> 💨 <\
🧠 Learnings used
Learnt from: userquin
Repo: npmx-dev/npmx.dev PR: 1170
File: app/components/Package/MetricsBadges.vue:66-66
Timestamp: 2026-02-08T15:02:02.232Z
Learning: In Vue components, use colon syntax for UnoCSS preset-icons (e.g., `i-carbon:checkmark` instead of `i-carbon-checkmark`). For long icon names with dashes, the colon syntax provides a performance optimization by helping UnoCSS resolve the collection directly without trying different combinations.
Learnt from: alexdln
Repo: npmx-dev/npmx.dev PR: 838
File: app/pages/package/[...package].vue:445-449
Timestamp: 2026-02-03T13:59:33.392Z
Learning: The copy button pattern in app/pages/package/[...package].vue may be made into a reusable component or pattern in the future, but currently it's acceptable to keep it inline with the CSS-only approach for smooth animations.
Learnt from: skaldebane
Repo: npmx-dev/npmx.dev PR: 760
File: app/components/Compare/FacetCard.vue:134-136
Timestamp: 2026-02-06T07:13:58.289Z
Learning: In `app/components/Compare/FacetCard.vue`, the facet values use `dir="auto"` rather than `dir="ltr"` because the values are not always numbers/sizes and can contain mixed content types.
# Conflicts: # app/components/DependencyPathPopup.vue
# Conflicts: # app/components/Package/Versions.vue
|
Can you provide some screenshots of before and after please? |
|
Just open nuxt package, go to old versions and select 0.18.0 (or 0.18.1 🤔 ), switch to light theme and check the package (red tip, orange collapsible and version numbers on right). For shiki check the template code snippet (green) and some red colors for ts code snippet. We need to fix some green tip colors.(check nuxt package tip with light and dark theme). Check also vulnerabilities orange number: just compare them with this pr preview and npmx.dev. |
4 participants
This PR includes:
utils/severity.tscolors including a darker color for light theme at severity and severity text colors constantsutils/npm/outdated-dependencies.tscolors including a darker color for light themeapp/components/Tag/Static.vueusing opacity-40: changed variant ghost to a lighter text color with a dashed border adding a gray solid border to the defaultapp/components/Package/VulnerabilityTree.vueusing divs inside buttonsserver/utils/shiki.tsto replace some red and green colors at github-light theme, we may need to review the the them at shikicloses #1167