test(utils): add vitest for utils package #2810
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request involves a comprehensive refactoring of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Walkthrough此PR为@opentiny/utils包增加了vitest测试用例，简化了log的实现，删除了nanoid代码，并简化了index的导出写法。这些改动旨在提高代码的可维护性和简洁性。 Changes
|
![petercat-assistant[bot]](https://avatars.githubusercontent.com/in/875736?s=60&v=4){kind=small}
| const html = `<a href="javascript:alert('XSS')">Click Me</a>` | ||
|
|
||
| // filterUrl是无效的？？？ | ||
| expect(filterUrl(url)).toMatchInlineSnapshot(`"https://s.com/user#<script>alert('XSS')</script>"`) |
There was a problem hiding this comment.
The filterUrl function seems to be ineffective as it does not sanitize the URL as expected. This could lead to potential XSS vulnerabilities if the function is used in a security-sensitive context. Please review the implementation of filterUrl to ensure it properly sanitizes URLs.
|
[e2e-test-warn] The title of the Pull request should look like "fix(vue-renderless): [action-menu, alert] fix xxx bug". Please make sure you've read our contributing guide |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
packages/utils/src/index.ts (1)
7-10: Consider removing default export to improve maintainability.Having both named and default exports can lead to confusion and inconsistent import patterns across the codebase. Consider removing the default export and sticking with named exports only, which is a TypeScript best practice.
-export default { - xss, - log, - crypt -}packages/utils/src/crypt/index.ts (1)
3-3: Add English translation to the Chinese comment.For better international collaboration, consider adding English translation alongside Chinese comments.
-/** 生成字节流或字符串的sha256编码 */ +/** + * Generate SHA-256 hash for byte stream or string + * 生成字节流或字符串的sha256编码 + */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/utils/src/crypt/__test__/__snapshots__/crypt.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (17)
packages/mobile/components/file-upload/src/renderless/index.ts(4 hunks)packages/renderless/src/common/index.ts(1 hunks)packages/renderless/src/tree/index.ts(1 hunks)packages/utils/README.md(1 hunks)packages/utils/package.json(2 hunks)packages/utils/src/crypt/__test__/crypt.test.ts(1 hunks)packages/utils/src/crypt/core.ts(0 hunks)packages/utils/src/crypt/index.ts(1 hunks)packages/utils/src/crypt/sha256.ts(0 hunks)packages/utils/src/index.ts(1 hunks)packages/utils/src/log/index.ts(1 hunks)packages/utils/src/logger/README.md(0 hunks)packages/utils/src/logger/index.ts(0 hunks)packages/utils/src/nanoid/README.md(0 hunks)packages/utils/src/nanoid/index.ts(0 hunks)packages/utils/src/nanoid/nanoid.ts(0 hunks)packages/utils/src/xss/__test__/xss.test.ts(1 hunks)
💤 Files with no reviewable changes (7)
- packages/utils/src/logger/README.md
- packages/utils/src/crypt/sha256.ts
- packages/utils/src/nanoid/README.md
- packages/utils/src/nanoid/nanoid.ts
- packages/utils/src/logger/index.ts
- packages/utils/src/crypt/core.ts
- packages/utils/src/nanoid/index.ts
✅ Files skipped from review due to trivial changes (3)
- packages/renderless/src/tree/index.ts
- packages/utils/README.md
- packages/mobile/components/file-upload/src/renderless/index.ts
🔇 Additional comments (3)
packages/renderless/src/common/index.ts (1)
13-13: LGTM!The removal of the logging implementation and simplification of imports aligns well with the PR objectives.
packages/utils/package.json (2)
19-20: Add Vitest configuration file.While the test script is added, a Vitest configuration file is needed to define test environment and settings.
Let's check if a vitest config exists:
If no config file exists, consider adding one with this content:
// vitest.config.ts import { defineConfig } from 'vitest/config' export default defineConfig({ test: { environment: 'node', include: ['src/**/*.{test,spec}.{js,mjs,cjs,ts,mts,cts,jsx,tsx}'] } })
3-3: Verify module type compatibility.The addition of
"type": "module"requires all .js files to use ESM syntax. Ensure all files are compatible with this change.Run this script to check for CommonJS syntax in .js files:
✅ Verification successful
ESM compatibility verified successfully ✅
The codebase is fully compatible with ESM modules:
- TypeScript is configured for ESM output (
"module": "ESNext")- All source files use ES module syntax
- No CommonJS patterns found in any files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for potential CommonJS syntax in .js files rg -l "require\(|module\.exports" "*.js"Length of output: 87
Script:
#!/bin/bash # Check for CommonJS syntax patterns in JavaScript files under packages/utils cd packages/utils && rg -l -g "*.{js,mjs,cjs}" -e "require\(" -e "module\.exports" -e "exports\." -e "__dirname" -e "__filename" # Show context if any matches found cd packages/utils && rg -A 2 -B 2 -g "*.{js,mjs,cjs}" "require\(|module\.exports|exports\.|__dirname|__filename"Length of output: 411
Script:
#!/bin/bash # First, find all JavaScript files echo "JavaScript files in packages/utils:" fd -e js -e mjs -e cjs . packages/utils # Then search for CommonJS patterns in found files echo -e "\nChecking for CommonJS patterns:" fd -e js -e mjs -e cjs . packages/utils -x rg -l -e "require\(" -e "module\.exports" -e "exports\." -e "__dirname" -e "__filename" {} || true # Show directory structure for debugging echo -e "\nDirectory structure:" ls -R packages/utils 2>/dev/null || trueLength of output: 861
Script:
#!/bin/bash # Check TypeScript configuration echo "TypeScript configuration:" cat packages/utils/tsconfig.json echo -e "\nChecking for ES module syntax in TypeScript files:" fd -e ts . packages/utils/src -x rg -l "^import |^export " {} echo -e "\nChecking for any potential CommonJS patterns in TypeScript files:" fd -e ts . packages/utils/src -x rg -l "require\(|module\.exports|exports\." {}Length of output: 1400
| test('测试sha256', async () => { | ||
| // 简单记录加密的结果，测试用来保证sha256算法不变化 | ||
| expect(await sha256('hello world')).toMatchSnapshot() | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage and clarity.
A few suggestions to improve the test suite:
- Consider using English for test descriptions to maintain consistency with code
- Instead of snapshot testing, explicitly verify the hash value for 'hello world'
- Add test cases for error scenarios (empty input, non-string input, etc.)
Example implementation:
test('sha256 should generate correct hash for known input', async () => {
const result = await sha256('hello world')
expect(result).toBe('b94d27b9934d3e08a52e52d7da7dabfac484efe37a5380ee9088f7ace2efcde9')
})
test('sha256 should handle empty string', async () => {
const result = await sha256('')
expect(result).toBe('e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855')
})
test('sha256 should reject non-string input', async () => {
await expect(sha256(123 as any)).rejects.toThrow()
})| import { getWindow } from '../window' | ||
|
|
||
| const _win: any = getWindow() | ||
| /** 使用 log.logger.xxx 代替 window.console.xxx, 避免语法警告 */ | ||
| export const log = { logger: _win.console as Console } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety and documentation.
Several improvements can be made:
- Avoid using
anytype for window object - Consider using English comments for consistency
- Add proper TypeScript types
import { getWindow } from '../window'
-const _win: any = getWindow()
-/** 使用 log.logger.xxx 代替 window.console.xxx, 避免语法警告 */
-export const log = { logger: _win.console as Console }
+/** Use log.logger.xxx instead of window.console.xxx to avoid syntax warnings */
+const win = getWindow()
+
+interface Logger {
+ logger: Console;
+}
+
+export const log: Logger = {
+ logger: win?.console ?? console
+}📝 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.
| import { getWindow } from '../window' | |
| const _win: any = getWindow() | |
| /** 使用 log.logger.xxx 代替 window.console.xxx, 避免语法警告 */ | |
| export const log = { logger: _win.console as Console } | |
| import { getWindow } from '../window' | |
| /** Use log.logger.xxx instead of window.console.xxx to avoid syntax warnings */ | |
| const win = getWindow() | |
| interface Logger { | |
| logger: Console; | |
| } | |
| export const log: Logger = { | |
| logger: win?.console ?? console | |
| } |
| test('测试 filterUrl,filterHtml, 整个组件库只用到这2个函数', async () => { | ||
| const { filterHtml, filterUrl } = xss.default | ||
|
|
||
| const url = `https://s.com/user#<script>alert('XSS')</script>` | ||
| const html = `<a href="javascript:alert('XSS')">Click Me</a>` | ||
|
|
||
| // filterUrl是无效的？？？ | ||
| expect(filterUrl(url)).toMatchInlineSnapshot(`"https://s.com/user#<script>alert('XSS')</script>"`) | ||
| expect(filterHtml(html)).toMatchInlineSnapshot(`"<a>Click Me</a>"`) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance XSS test coverage.
The current test suite needs improvement:
- Use English for test descriptions
- Add more test cases covering various XSS attack vectors
- Use explicit assertions instead of snapshots for security-critical functions
- Add negative test cases
Example implementation:
describe('XSS Protection', () => {
test('filterUrl should sanitize script tags in URLs', () => {
const cases = [
{
input: 'https://example.com/<script>alert("XSS")</script>',
expected: 'https://example.com/'
},
{
input: 'javascript:alert("XSS")',
expected: ''
}
]
cases.forEach(({ input, expected }) => {
expect(filterUrl(input)).toBe(expected)
})
})
test('filterHtml should remove dangerous attributes and tags', () => {
const cases = [
{
input: '<a href="javascript:alert(\'XSS\')">Click</a>',
expected: '<a>Click</a>'
},
{
input: '<img src="x" onerror="alert(\'XSS\')">',
expected: '<img src="x">'
}
]
cases.forEach(({ input, expected }) => {
expect(filterHtml(input)).toBe(expected)
})
})
})| // filterUrl是无效的？？？ | ||
| expect(filterUrl(url)).toMatchInlineSnapshot(`"https://s.com/user#<script>alert('XSS')</script>"`) |
There was a problem hiding this comment.
💡 Codebase verification
Security Vulnerability: Insufficient URL Sanitization in filterUrl
The filterUrl function has a critical security flaw that allows XSS payloads to pass through in URL components (path, query, fragment). The implementation only checks for malicious protocols but fails to properly escape HTML/script tags.
- Found in:
packages/utils/src/xss/index.ts - Impact: High severity due to widespread usage (50+ locations) in security-critical operations
- Example exploit: URLs with script tags in fragments pass through unmodified, enabling XSS attacks
🔗 Analysis chain
Security concern: filterUrl appears to be ineffective.
The comment and test result suggest that filterUrl is not sanitizing the XSS payload. This is a potential security vulnerability that needs immediate attention.
Let's verify the XSS sanitization implementation:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for filterUrl implementation and its usages
ast-grep --pattern 'function filterUrl($_) {
$$$
}'
# Find all usages of filterUrl in the codebase
rg 'filterUrl\(' -A 2
Length of output: 16857
Script:
#!/bin/bash
# Find filterUrl implementation
rg -l 'filterUrl' packages/utils/src/xss/
cat packages/utils/src/xss/index.ts
Length of output: 5307
| export async function sha256(message: ArrayBuffer | string) { | ||
| const isArrayBuffer = Object.prototype.toString.call(message) === '[object ArrayBuffer]' | ||
| const msgUint8 = isArrayBuffer ? message : new TextEncoder().encode(message) // 编码为（utf-8）Uint8Array | ||
| const hashBuffer = await window.crypto.subtle.digest(algo, msgUint8) // 计算消息的哈希值 | ||
| const msgUint8 = isArrayBuffer ? message : new TextEncoder().encode(message as string) // 编码为（utf-8）Uint8Array | ||
| const hashBuffer = await getWindow().crypto.subtle.digest('SHA-256', msgUint8) // 计算消息的哈希值 | ||
| const hashArray = Array.from(new Uint8Array(hashBuffer)) // 将缓冲区转换为字节数组 | ||
| const hashHex = hashArray.map((b) => b.toString(16).padStart(2, '0')).join('') // 将字节数组转换为十六进制字符串 | ||
|
|
||
| return hashHex |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for crypto API failures.
The sha256 function should handle potential errors from crypto.subtle.digest() as it may fail in certain environments or with invalid inputs.
Consider wrapping the crypto operations in try-catch:
export async function sha256(message: ArrayBuffer | string) {
const isArrayBuffer = Object.prototype.toString.call(message) === '[object ArrayBuffer]'
const msgUint8 = isArrayBuffer ? message : new TextEncoder().encode(message as string)
- const hashBuffer = await getWindow().crypto.subtle.digest('SHA-256', msgUint8)
- const hashArray = Array.from(new Uint8Array(hashBuffer))
- const hashHex = hashArray.map((b) => b.toString(16).padStart(2, '0')).join('')
- return hashHex
+ try {
+ const hashBuffer = await getWindow().crypto.subtle.digest('SHA-256', msgUint8)
+ const hashArray = Array.from(new Uint8Array(hashBuffer))
+ const hashHex = hashArray.map((b) => b.toString(16).padStart(2, '0')).join('')
+ return hashHex
+ } catch (error) {
+ throw new Error(`SHA-256 hashing failed: ${error.message}`)
+ }
}📝 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.
| export async function sha256(message: ArrayBuffer | string) { | |
| const isArrayBuffer = Object.prototype.toString.call(message) === '[object ArrayBuffer]' | |
| const msgUint8 = isArrayBuffer ? message : new TextEncoder().encode(message) // 编码为（utf-8）Uint8Array | |
| const hashBuffer = await window.crypto.subtle.digest(algo, msgUint8) // 计算消息的哈希值 | |
| const msgUint8 = isArrayBuffer ? message : new TextEncoder().encode(message as string) // 编码为（utf-8）Uint8Array | |
| const hashBuffer = await getWindow().crypto.subtle.digest('SHA-256', msgUint8) // 计算消息的哈希值 | |
| const hashArray = Array.from(new Uint8Array(hashBuffer)) // 将缓冲区转换为字节数组 | |
| const hashHex = hashArray.map((b) => b.toString(16).padStart(2, '0')).join('') // 将字节数组转换为十六进制字符串 | |
| return hashHex | |
| export async function sha256(message: ArrayBuffer | string) { | |
| const isArrayBuffer = Object.prototype.toString.call(message) === '[object ArrayBuffer]' | |
| const msgUint8 = isArrayBuffer ? message : new TextEncoder().encode(message as string) // 编码为（utf-8）Uint8Array | |
| try { | |
| const hashBuffer = await getWindow().crypto.subtle.digest('SHA-256', msgUint8) | |
| const hashArray = Array.from(new Uint8Array(hashBuffer)) | |
| const hashHex = hashArray.map((b) => b.toString(16).padStart(2, '0')).join('') | |
| return hashHex | |
| } catch (error) { | |
| throw new Error(`SHA-256 hashing failed: ${error.message}`) | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/vue/src/grid/src/tools/logger.ts(2 hunks)
🔇 Additional comments (2)
packages/vue/src/grid/src/tools/logger.ts (2)
Line range hint
1-18: LGTM! Clean and well-structured logging implementation.The overall implementation is well-designed with:
- Factory pattern for different log levels
- Consistent message formatting with '[tiny-grid]' prefix
- I18n support through GlobalConfig
- Clear separation between warning and error levels
1-1: Verify the new import path across the codebase.The import source has changed from
@opentiny/vue-renderless/commonto@opentiny/utils, which aligns with the PR objective of streamlining the utils package.Let's verify this change is consistent across the codebase:
✅ Verification successful
Import change is properly implemented and consistent
The change from '@opentiny/vue-renderless/common' to '@opentiny/utils' is correctly implemented:
- No remaining old imports found in the codebase
- New log utility provides equivalent console logging functionality
- Implementation maintains all required features including i18n support and message formatting
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining imports from the old path and verify the new import exists # Check for any remaining old imports echo "Checking for remaining old imports..." rg "from '@opentiny/vue-renderless/common'" -l # Verify the new log export exists in @opentiny/utils echo "Verifying new export exists..." rg "export.*log" "packages/utils/"Length of output: 438
Script:
#!/bin/bash # Check the implementation and usage echo "Checking log implementation..." cat packages/utils/src/log/index.ts echo -e "\nChecking grid component usage..." cat packages/vue/src/grid/src/tools/logger.tsLength of output: 792
| @@ -8,7 +8,7 @@ const outLog = (type) => (message, detail) => { | |||
| msg += `: ${detail}` | |||
| } | |||
|
|
|||
| log(msg, type) | |||
| log.logger.log(msg, type) | |||
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent and broken logging implementation found
The verification revealed issues with the logging implementation:
- The currency component imports
logfrom '../common' which doesn't export any logging functionality - This differs from the official logging interface from '@opentiny/utils' used in the grid component
The currency component should be updated to use the official logging interface:
import { log } from '@opentiny/utils'🔗 Analysis chain
Verify the new logging interface structure.
The change from log(msg, type) to log.logger.log(msg, type) suggests a new structured logging interface. While this change aligns with the PR's goal of simplifying logging implementation, we should verify the new interface is consistently used.
Let's check for consistency:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the new logging interface usage
# Check for any remaining direct log calls
echo "Checking for old logging pattern..."
rg "log\(.*,.*\)" --type ts
# Check for new logging pattern usage
echo "Checking new logging pattern usage..."
rg "log\.logger\.log" --type ts
Length of output: 1449
Script:
#!/bin/bash
# Check implementation details of both logging patterns
echo "=== Grid Logger Implementation ==="
cat packages/vue/src/grid/src/tools/logger.ts
echo -e "\n=== Currency Module Implementation ==="
cat packages/renderless/src/currency/index.ts
echo -e "\n=== Checking imports in currency module ==="
rg "import.*log" packages/renderless/src/currency/index.ts
Length of output: 5069
Script:
#!/bin/bash
# Check the common module implementation
echo "=== Common Module Implementation ==="
cat packages/renderless/src/common/index.ts
# Also check if there's a dedicated logging file
echo -e "\n=== Looking for logging-related files in common ==="
fd "log" packages/renderless/src/common/
Length of output: 6565
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
New Features
@opentiny/utilspackage.Refactoring
@opentiny/utils.Testing
Chores