fix: code quality and safety improvements - #3377
Open
saurabhhhcodes wants to merge 1 commit into
Open
Conversation
github-actions
Bot
added
type:bug
📝 WalkthroughWalkthroughThe PR replaces default lexicographic sorting with explicit numeric comparators in CLI dependency handling, token serialization, multi-select ordering, and a JSX ChangesNumeric sorting updates
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/jsx/src/hooks.ts (1)
482-482: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCopy
itemsbefore sorting.
items.sort(...)mutates the input array. Ifitemscomes from props or state, the example mutates caller-owned data during memoization.Use a copy before sorting:
- * const sorted = useMemo(() => items.sort((a, b) => a - b), [items]); + * const sorted = useMemo(() => [...items].sort((a, b) => a - b), [items]);🤖 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 `@packages/jsx/src/hooks.ts` at line 482, Update the useMemo sorting example in hooks.ts to copy items before calling sort, preventing mutation of the caller-owned array while preserving the existing numeric sort behavior.packages/ui/src/MultiSelect.ts (1)
33-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an ordering regression test.
packages/ui/src/MultiSelect.test.ts:94-102selects only indices 0, 1, and 2. That test passes with both numeric and lexicographic ordering. Add a case with indices such as 2 and 10, then assert the returned option order.🤖 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 `@packages/ui/src/MultiSelect.ts` at line 33, Add an ordering regression test in MultiSelect.test.ts covering selected indices such as 2 and 10, then assert that the returned options follow numeric index order. Keep the existing selection behavior and use the MultiSelect API exercised by the nearby tests.
🤖 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 `@packages/cli/src/registry.ts`:
- Line 70: Replace the numeric a - b comparators used to sort string identifiers
with deterministic lexicographic string sorting. Apply this to
packages/cli/src/registry.ts lines 70-70, packages/cli/src/commands/add.ts lines
83-83, and packages/tss/src/tokens.ts lines 105-105, covering dependency names,
allDeps, and token keys without changing the surrounding sorting behavior.
---
Nitpick comments:
In `@packages/jsx/src/hooks.ts`:
- Line 482: Update the useMemo sorting example in hooks.ts to copy items before
calling sort, preventing mutation of the caller-owned array while preserving the
existing numeric sort behavior.
In `@packages/ui/src/MultiSelect.ts`:
- Line 33: Add an ordering regression test in MultiSelect.test.ts covering
selected indices such as 2 and 10, then assert that the returned options follow
numeric index order. Keep the existing selection behavior and use the
MultiSelect API exercised by the nearby tests.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3d86198b-eebf-47a9-929e-c6817af4ac1a
📒 Files selected for processing (5)
packages/cli/src/commands/add.tspackages/cli/src/registry.tspackages/jsx/src/hooks.tspackages/tss/src/tokens.tspackages/ui/src/MultiSelect.ts
| ...(json.peerDeps ?? []), | ||
| ]), | ||
| ].sort(); | ||
| ].sort((a, b) => a - b); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
if rg -n 'sort\(\(a, b\) => a - b\)' \
packages/cli/src/commands/add.ts \
packages/cli/src/registry.ts \
packages/tss/src/tokens.ts
then
echo "Invalid numeric comparator remains" >&2
exit 1
fi
npx tsc --noEmit --pretty falseRepository: Karanjot786/TermUI
Length of output: 437
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== file snippets =="
sed -n '60,76p' packages/cli/src/registry.ts | cat -n
printf '\n'
sed -n '76,90p' packages/cli/src/commands/add.ts | cat -n
printf '\n'
sed -n '95,112p' packages/tss/src/tokens.ts | cat -n
printf '\n== TypeScript config strictness indicators ==\n'
for f in tsconfig.json packages/*/tsconfig*.json; do
[ -f "$f" ] || continue
printf '%s\n' "$f"
python3 - <<'PY' "$f"
import json, sys
p=sys.argv[1]
try:
data=json.load(open(p))
except Exception as e:
print("JSON/read error:", e); raise SystemExit
print(json.dumps(data.get("compilerOptions", {}), indent=2)[:800].replace("\n"," "))
PY
done
printf '\n== runtime numeric-subtraction behavior for representative strings =="
node - <<'JS'
const samples = [['a','b'], ['a', '42'], ['10', '2'], undefined, null, {}, []]
for (const [a, b] of samples) console.log(`${JSON.stringify(a)} - ${JSON.stringify(b)} = ${a - b}`)
JS
printf '\n== type assignment check if tsc runs without repo code ==\n'
npm_exec="$(cd "$(dirname "$(command -v npm)")" && pwd -P)/$(basename "$(command -v npm)")"
"$npm_exec" exec --package typescript -- tsc --noEmit --strict --declaration --skipLibCheck --allowJs false <<'TS'
type T = string[];
const deps = ['a', '10'];
deps.sort((a, b) => a - b);
TSRepository: Karanjot786/TermUI
Length of output: 5048
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== TSConfig inheritance references =="
for f in tsconfig.json packages/cli/tsconfig.json packages/tss/tsconfig.json packages/tsconfig.*.json; do
[ -f "$f" ] || continue
printf '%s\n' "$f"
node -e "const fs=require('node:fs'); const j=JSON.parse(fs.readFileSync(process.argv[1],'utf8')); console.log(JSON.stringify(j, null, 2))" "$f"
done
printf '\n== TypeScript numeric-subtraction assignment check (standalone) ==\n'
npm exec --package typescript -- tsc --noEmit --strict --declaration --skipLibCheck --allowJs false <<'TS'
type T = string[];
const deps: T = ['a', '10'];
deps.sort((a, b) => a - b);
TS
printf '\n== Runtime numeric-subtraction behavior for representative strings ==\n'
node - <<'JS'
const samples = [['a','b'], ['a', '42'], ['10', '2'], undefined, null, {}, []]
for (const [a, b] of samples) console.log(`${JSON.stringify(a)} - ${JSON.stringify(b)} = ${a - b}`)
JSRepository: Karanjot786/TermUI
Length of output: 6329
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
npm_exec="$(node -e "const path=require('node:path'); const which=process.env.npm_execpath || path.join(process.cwd(), process.env.npm_config_user_prefix || '.', 'bin/npm'); console.log(which)")"
"$npm_exec" exec --package typescript -- tsc --noEmit --strict --declaration --skipLibCheck --allowJs false <<'TS'
type T = string[];
const deps: T = ['a', '10'];
deps.sort((a, b) => a - b);
TS
node - <<'JS'
const samples = [['a','b'], ['a', '42'], ['10', '2'], undefined, null, {}, []]
for (const [a, b] of samples) console.log(`${JSON.stringify(a)} - ${JSON.stringify(b)} = ${a - b}`)
JSRepository: Karanjot786/TermUI
Length of output: 228
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
tmp="$(mktemp -d)"
cd "$tmp"
cat > tsconfig.json <<'JS'
{
"compilerOptions": {
"target": "ES2022",
"module": "ESNext",
"strict": true,
"skipLibCheck": true
}
}
JS
cat > test.ts <<'TS'
type T = string[];
const deps: T = ['a', '10'];
deps.sort((a, b) => a - b);
TS
npm exec --package typescript -- tsc --noEmit test.ts || true
node - <<'JS'
const samples = [['a','b'], ['a', '42'], ['10', '2'], undefined, null, {}, []]
for (const [a, b] of samples) console.log(`${JSON.stringify(a)} - ${JSON.stringify(b)} = ${a - b}`)
JSRepository: Karanjot786/TermUI
Length of output: 1325
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
tmp="$(mktemp -d)"
cd "$tmp"
cat > tsconfig.json <<'JS'
{
"compilerOptions": {
"target": "ES2022",
"module": "ESNext",
"strict": false,
"skipLibCheck": true
}
}
JS
cat > test.ts <<'TS'
type T = string[];
const deps: T = ['a', '10'];
deps.sort((a, b) => a - b);
TS
node - <<'JS'
const samples = [['a','b'], ['a', '42'], ['10', '2'], undefined, null, {}, []]
for (const [a, b] of samples) console.log(`${JSON.stringify(a)} - ${JSON.stringify(b)} = ${String(a - b)}`)
JS
node - <<'TS'
const files = ['test.ts']
const ts = await import('typescript')
const host = ts.createCompilerHost({
...ts.getDefaultCompilerOptions(),
strict: false,
skipLibCheck: true,
})
host.readFile = (file) => file === '/tmp/test.ts' ? 'type T = string[];\nconst deps: T = [\'a\', \'10\'];\ndeps.sort((a, b) => a - b);\n' : undefined
const prog = ts.createProgram(files, ts.getDefaultCompilerOptions(), host)
const diags = ts.getPreEmitDiagnostics(prog)
console.log(diags.map(d => `${d.file ? d.file.fileName : ''}${d.start ?? ''}:${d.length ?? ''} TS${d.code} ${d.messageText}`).join('\n'))
TSRepository: Karanjot786/TermUI
Length of output: 879
Replace numeric comparators on string identifiers.
a - b should not sort dependency names, allDeps, or token keys. These values are strings, so the expression coerces unsupported strings to NaN; use deterministic string sorting instead.
📍 Affects 3 files
packages/cli/src/registry.ts#L70-L70(this comment)packages/cli/src/commands/add.ts#L83-L83packages/tss/src/tokens.ts#L105-L105
🤖 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 `@packages/cli/src/registry.ts` at line 70, Replace the numeric a - b
comparators used to sort string identifiers with deterministic lexicographic
string sorting. Apply this to packages/cli/src/registry.ts lines 70-70,
packages/cli/src/commands/add.ts lines 83-83, and packages/tss/src/tokens.ts
lines 105-105, covering dependency names, allDeps, and token keys without
changing the surrounding sorting behavior.
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