chore(themes): add additional tests for css variable completions#127
Open
coryrylan wants to merge 1 commit into
Open
chore(themes): add additional tests for css variable completions#127coryrylan wants to merge 1 commit into
coryrylan wants to merge 1 commit into
Conversation
github-actions
Bot
added
scope(themes)
dependencies
📝 WalkthroughWalkthroughThis PR refactors the themes build scripts from side-effect modules into reusable ESM-compatible modules, extracting internal helpers into exported functions, implementing explicit cycle detection for token resolution, and adding comprehensive test coverage integrated into the build and CI pipeline. ChangesBuild Script Modularization and Testing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Signed-off-by: Cory Rylan <crylan@nvidia.com>
coryrylan
force-pushed
the
topic-cleanup-themes
branch
from
57bb039 to
20eb61b
Compare
![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
🤖 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.
Inline comments:
In `@projects/themes/build/css-var-completions.js`:
- Around line 167-187: The code constructs file paths using template literals
with sourcePath and buildPath (used in buildCssVarCompletions,
loadTokenDictionary calls and fs.mkdirSync/writeJSONFile) while sourcePath is
defined with a trailing slash, producing double slashes; change all path
constructions to use path.join (e.g., replace `${sourcePath}/index.json` etc.)
or normalize by removing trailing slashes from sourcePath and buildPath and
updating calls to loadTokenDictionary, fs.existsSync/fs.mkdirSync, and
writeJSONFile to use the normalized paths so you never emit '//'. Ensure you
import/require path if switching to path.join.
In `@projects/themes/package.json`:
- Line 49: The project currently lists the devDependency
"`@vitest/coverage-istanbul`" but the Vitest invocation in
projects/themes/package.json doesn't pass --coverage, so coverage
provider/config in projects/themes/vitest.config.ts (which merges
libraryNodeTestConfig) never runs; either add --coverage to the Vitest CLI used
in the package.json script that runs Vitest so the existing coverage
configuration in vitest.config.ts is applied, or if you don't intend to run
coverage from this package remove the "`@vitest/coverage-istanbul`" entry; locate
the Vitest-running script in projects/themes/package.json and update it (or
remove the dependency) accordingly, keeping vitest.config.ts and
libraryNodeTestConfig intact.
In `@projects/themes/src/index.test.ts`:
- Around line 7-9: The helper function readThemeCss lacks an explicit return
type; update its signature to declare the return type (e.g. async function
readThemeCss(theme: string): Promise<string>) so TypeScript knows it returns a
promise of a string, leaving the body using readFile as-is; ensure the
Promise<string> annotation matches the value returned by readFile in this
function.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: acdeee5b-75cf-4f09-a9b1-f4fd6bf770ac
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
projects/themes/build/css-var-completions.jsprojects/themes/build/css-var-completions.test.jsprojects/themes/build/style-dictionary.config.jsprojects/themes/build/style-dictionary.config.test.jsprojects/themes/package.jsonprojects/themes/src/index.test.tsprojects/themes/vitest.config.ts
Comment on lines
+167
to
187
| export function buildCssVarCompletions() { | ||
| const baseTokenDictionary = loadTokenDictionary(`${sourcePath}/index.json`); | ||
| const compactThemeTokenDictionary = loadTokenDictionary(`${sourcePath}/compact.json`); | ||
| const darkThemeTokenDictionary = loadTokenDictionary(`${sourcePath}/dark.json`); | ||
| const highContrastThemeTokenDictionary = loadTokenDictionary(`${sourcePath}/high-contrast.json`); | ||
| const reducedMotionThemeTokenDictionary = loadTokenDictionary(`${sourcePath}/reduced-motion.json`); | ||
|
|
||
| const cssVarCompletions = createCssVarCompletions({ | ||
| baseTokenDictionary, | ||
| compactThemeTokenDictionary, | ||
| darkThemeTokenDictionary, | ||
| highContrastThemeTokenDictionary, | ||
| reducedMotionThemeTokenDictionary | ||
| }); | ||
|
|
||
| if (!fs.existsSync(`${buildPath}`)) { | ||
| fs.mkdirSync(`${buildPath}`); | ||
| } | ||
|
|
||
| writeJSONFile('./dist/data.css-vars.json', cssVarCompletions); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Inconsistent path construction with sourcePath.
sourcePath is defined as 'src/' (line 7) with a trailing slash. Using template literals like `${sourcePath}/index.json` produces 'src//index.json'. While Node.js tolerates double slashes, this is inconsistent. Consider removing the trailing slash from sourcePath and buildPath, or use path.join() for all path construction.
Suggested path consistency fix
-const buildPath = 'dist/';
-const sourcePath = 'src/';
+const buildPath = 'dist';
+const sourcePath = 'src';Then update usages accordingly, or use path.join(sourcePath, 'index.json') etc.
🤖 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 `@projects/themes/build/css-var-completions.js` around lines 167 - 187, The
code constructs file paths using template literals with sourcePath and buildPath
(used in buildCssVarCompletions, loadTokenDictionary calls and
fs.mkdirSync/writeJSONFile) while sourcePath is defined with a trailing slash,
producing double slashes; change all path constructions to use path.join (e.g.,
replace `${sourcePath}/index.json` etc.) or normalize by removing trailing
slashes from sourcePath and buildPath and updating calls to loadTokenDictionary,
fs.existsSync/fs.mkdirSync, and writeJSONFile to use the normalized paths so you
never emit '//'. Ensure you import/require path if switching to path.join.
| ], | ||
| "devDependencies": { | ||
| "@internals/vite": "workspace:*", | ||
| "@vitest/coverage-istanbul": "catalog:", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check for coverage configuration in vitest configs and shared test setup
# Check for coverage configuration in local and shared configs
rg -n "coverage" vitest.config.ts
rg -n "coverage" projects/internals/vite/src/configs/test.node.jsRepository: NVIDIA/elements
Length of output: 514
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Find actual vitest config files
fd -t f -i 'vitest.config.ts' . || true
fd -t f -i '*vitest*.ts' projects . || true
# 2) Search for `@vitest/coverage-istanbul` usage
rg -n "`@vitest/coverage-istanbul`" . || true
# 3) Search for coverage config in likely Vitest config files
rg -n "coverage\s*:" projects/internals/vite/src/configs -S || true
rg -n "reportsDirectory|reporter|lcov|html|json-summary" projects/internals/vite/src/configs -S || true
# 4) Show how coverage is wired in shared configs (the file from initial run)
ls -la projects/internals/vite/src/configs || true
sed -n '1,120p' projects/internals/vite/src/configs/test.node.js || trueRepository: NVIDIA/elements
Length of output: 17052
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect themes vitest config
echo "=== projects/themes/vitest.config.ts ==="
sed -n '1,200p' projects/themes/vitest.config.ts || true
# Check for usage of shared libraryNodeTestConfig in themes config
echo "=== rg libraryNodeTestConfig usage (themes) ==="
rg -n "libraryNodeTestConfig" projects/themes || true
# Inspect themes package scripts for coverage usage
echo "=== projects/themes/package.json (scripts + vitest/coverage) ==="
rg -n "\"scripts\"" -n projects/themes/package.json || true
sed -n '1,220p' projects/themes/package.json || true
# Also search if themes config passes --coverage
echo "=== rg \"--coverage\" in themes ==="
rg -n "--coverage" projects/themes || trueRepository: NVIDIA/elements
Length of output: 6430
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find places that pass coverage to vitest (limit scope to repo)
rg -n "vitest(\s+run)?[^\n]*--coverage" . || true
rg -n "(^|\s)--coverage(\s|$)" projects -S || true
# Also check wireit internals/vite:ci for themes
rg -n "internals/vite:ci" -S projects/themes/package.json projects -g'*.json' || true
# Search for "coverage" in wireit scripts around vitest
rg -n "wireit" projects/internals/vite -S || true
rg -n "coverage" projects/internals/vite -S || trueRepository: NVIDIA/elements
Length of output: 7482
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,140p' projects/internals/vite/package.json || true
rg -n "\"ci\"" projects/internals/vite/package.json -n || true
# Show wireit section for ci task(s)
rg -n "wireit" -n projects/internals/vite/package.json || true
# Search for vitest command templates inside internals/vite
rg -n "vitest run" projects/internals/vite -S || true
rg -n "--coverage" projects/internals/vite -S || true
# If there is a wireit config file or scripts, find them
fd -t f projects/internals/vite -i 'wireit*' || true
fd -t f projects/internals/vite -i '*config*' || trueRepository: NVIDIA/elements
Length of output: 2626
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== rg coverage in projects/themes/package.json ==="
rg -n "coverage" projects/themes/package.json || true
echo "=== list themes vitest.* configs ==="
ls -ლა projects/themes/vitest* 2>/dev/null || true
fd -t f -i "vitest*.ts" projects/themes || true
echo "=== rg --coverage in projects/themes ==="
rg -n "--coverage" projects/themes || true
echo "=== rg coverage in projects/themes/vitest*.ts ==="
rg -n "coverage" projects/themes/vitest*.ts || trueRepository: NVIDIA/elements
Length of output: 882
Coverage tooling is already configured via the shared Vitest config
projects/themes/vitest.config.tsmergeslibraryNodeTestConfigfromprojects/internals/vite/src/configs/test.node.js, which already definescoverage(provideristanbul, reports/thresholds, etc.), so@vitest/coverage-istanbulis intentional for coverage runs.projects/themes/package.jsonrunsvitest run --config=vitest.config.ts(and lighthouse/visual) without--coverage, so coverage reports/threshold enforcement will only happen when Vitest is invoked with--coveragein CI or a follow-up task.
🤖 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 `@projects/themes/package.json` at line 49, The project currently lists the
devDependency "`@vitest/coverage-istanbul`" but the Vitest invocation in
projects/themes/package.json doesn't pass --coverage, so coverage
provider/config in projects/themes/vitest.config.ts (which merges
libraryNodeTestConfig) never runs; either add --coverage to the Vitest CLI used
in the package.json script that runs Vitest so the existing coverage
configuration in vitest.config.ts is applied, or if you don't intend to run
coverage from this package remove the "`@vitest/coverage-istanbul`" entry; locate
the Vitest-running script in projects/themes/package.json and update it (or
remove the dependency) accordingly, keeping vitest.config.ts and
libraryNodeTestConfig intact.
Comment on lines
+7
to
+9
| async function readThemeCss(theme: string) { | ||
| return readFile(new URL(`../dist/${theme}.css`, import.meta.url), 'utf8'); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Add explicit return type annotation.
The readThemeCss helper function should explicitly declare its return type for better type safety and code clarity. As per coding guidelines, TypeScript code should follow type safety best practices.
📝 Suggested improvement
-async function readThemeCss(theme: string) {
+async function readThemeCss(theme: string): Promise<string> {
return readFile(new URL(`../dist/${theme}.css`, import.meta.url), 'utf8');
}🤖 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 `@projects/themes/src/index.test.ts` around lines 7 - 9, The helper function
readThemeCss lacks an explicit return type; update its signature to declare the
return type (e.g. async function readThemeCss(theme: string): Promise<string>)
so TypeScript knows it returns a promise of a string, leaving the body using
readFile as-is; ensure the Promise<string> annotation matches the value returned
by readFile in this function.
Source: Coding guidelines
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
1 participant
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.
Summary by CodeRabbit