Repository navigation
fix: code quality and safety improvements - #3357
Closed
saurabhhhcodes wants to merge 1 commit into
Closed
saurabhhhcodes wants to merge 1 commit into
saurabhhhcodes wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThe changes replace default lexicographic sorting with numeric comparators in token compilation and a ChangesNumeric sorting updates
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 (1)
packages/jsx/src/hooks.ts (1)
482-482: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCopy
itemsbefore sorting.
items.sort(...)mutates the array. Sort a copied array to avoid changing caller-owned item data.Proposed fix
- * 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, ensuring the caller-owned array is not mutated while preserving the existing numeric sort behavior.
🤖 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/tss/src/tokens.ts`:
- Line 105: Update the comparator in the Object.keys(merged) sorting loop to
avoid numeric subtraction on string token identifiers. Parse and numerically
compare identifiers when both are numeric-looking, then use lexicographic string
comparison as the fallback for names such as accent, bg, and fg.
---
Nitpick comments:
In `@packages/jsx/src/hooks.ts`:
- Line 482: Update the useMemo sorting example in hooks.ts to copy items before
calling sort, ensuring the caller-owned array is not mutated while preserving
the existing numeric sort behavior.
🪄 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: 1e24adaf-a9b3-43e0-a04b-cebe9bb0e108
📒 Files selected for processing (2)
packages/jsx/src/hooks.tspackages/tss/src/tokens.ts
|
|
||
| const tokens: Record<string, string> = {}; | ||
| for (const key of Object.keys(merged).sort()) { | ||
| for (const key of Object.keys(merged).sort((a, b) => a - b)) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate tokens.ts:"
fd 'tokens\.ts$' . | sed -n '1,40p'
echo
echo "Relevant tokens.ts section if present:"
if [ -f packages/tss/src/tokens.ts ]; then
wc -l packages/tss/src/tokens.ts
sed -n '1,140p' packages/tss/src/tokens.ts | cat -n
fi
echo
echo "Search normalizeThemeVariables usage:"
rg -n "normalizeThemeVariables|Object\.keys\(merged\)\.sort|for \(const key of Object\.keys\(merged\)\.sort" packages/tss packages -g '*.ts' -g '*.tsx' || true
echo
echo "TypeScript availability/version:"
node - <<'JS'
try {
const ts = require('typescript');
console.log('typescript', ts.version);
} catch (e) {
console.log('typescript not available');
}
JS
echo
echo "Behavioral probe: string keys subtracted/sorted:"
node - <<'JS'
const merged = { accent: '`#f00`', bg: '`#000`', fg: '`#fff`' };
const keys = Object.keys(merged);
const before = [...keys];
const after = keys.sort((a, b) => a - b);
console.log({ before, after, numericSubtraction: { accentFgBefore: Math.acos(), accentFgAfter: Math.acos() } });
const pairs = keys.map(e => [e, Number(e), isNaN(Number(e))]);
console.log({ numericConversionMap: pairs, sortWithNumericFallbackArrayLiteral: [...keys].sort((a, b) => Number(a) - Number(b)).map(k => [k, isNaN(Number(k))]) });
JSRepository: Karanjot786/TermUI
Length of output: 6723
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Package files:"
fd -a 'package\.json|tsconfig.*\.json' . | sed -n '1,60p'
echo
echo "Root/package.json tsconfig references:"
for f in package.json packages/package.json packages/tss/package.json; do
if [ -f "$f" ]; then
echo "--- $f"
sed -n '1,160p' "$f" | sed -n '/typescript\|compilerOptions\|extends\|references\|scripts/p'
fi
done
echo
echo "Token-related test files:"
fd -e ts -e js -e tsx . packages tests test __tests__ | rg 'token|TSS|theme|tokens' || true
echo
echo "Behavioral probe with number-like keys:"
node - <<'JS'
const cases = [
{ '10': 'ten', '2': 'two' },
{ 'var-10': 'ten', 'var-2': 'two' },
];
for (const merged of cases) {
const keys = Object.keys(merged);
const sortNumeric = [...keys].sort((a, b) => a - b);
const sortFallback = [...keys].sort((a, b) => {
const numericA = Number(a);
const numericB = Number(b);
if (Number.isFinite(numericA) && Number.isFinite(numericB)) {
return numericA - numericB;
}
return a.localeCompare(b);
});
console.log({ input: merged, sortNumeric, sortFallback });
}
JSRepository: Karanjot786/TermUI
Length of output: 4641
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Relevant token-json tests:"
sed -n '1,220p' packages/tss/src/token-json.test.ts | cat -n
echo
echo "Relevant token tests:"
sed -n '1,240p' packages/tss/src/tokens.test.ts | cat -nRepository: Karanjot786/TermUI
Length of output: 5310
Use a type-safe comparator with a nonnumeric fallback.
Token identifiers are strings, so a - b is invalid in strict TypeScript and nonnumeric keys sort as NaN at runtime. Sort numeric-looking identifiers as numbers, then fall back to string comparison for names like accent, bg, and fg.
🤖 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/tss/src/tokens.ts` at line 105, Update the comparator in the
Object.keys(merged) sorting loop to avoid numeric subtraction on string token
identifiers. Parse and numerically compare identifiers when both are
numeric-looking, then use lexicographic string comparison as the fallback for
names such as accent, bg, and fg.
Source: Coding guidelines
This was referenced Aug 2, 2026
Closed
Closed
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
Documentation
useMemoexample to demonstrate reliable numeric sorting.Bug Fixes