| name | 1k-code-review-pr |
| description | Structured PR code review checklist for OneKey monorepo. Use when reviewing PRs to identify build issues, script problems, CI workflow gaps, and documentation inconsistencies. Focuses on actionable feedback with priority levels and specific fix suggestions. 代码审查. Code Review PR. |
| disable-model-invocation | true |
| allowed-tools | Read, Grep, Glob, Bash, WebFetch |
OneKey PR Code Review
输出语言: 使用中文输出所有审查报告内容。
This skill provides a structured approach to reviewing PRs in the OneKey monorepo, focusing on practical issues that can break builds, cause runtime errors, or lead to maintenance problems.
Review Scope
- Base branch:
x (main branch)
- Use triple-dot diff:
git fetch origin && git diff origin/x...HEAD
Relationship with pr-review Skill
This skill complements the existing pr-review skill:
| Aspect | pr-review | 1k-code-review-pr |
|---|
| Focus | Security & supply-chain risk | Build reliability & runtime quality |
| Best for | Dependency updates, auth changes, sensitive data | Business logic, UI components, scripts |
| Depth | Deep security analysis | Practical code patterns |
Recommendation: For complex PRs, use both skills for comprehensive coverage.
Review Checklist
1. Accidental File Commits (HIGH PRIORITY)
Check for files that shouldn't be in the repository:
git diff origin/x...HEAD --name-only | grep -E "^[^/]+$" | grep -v -E "\.(md|json|js|ts|yml|yaml)$"
git diff origin/x...HEAD --name-only | grep -E "(\.DS_Store|Thumbs\.db|\.env\.local|node_modules|\.log$)"
Report Format:
## 问题: 意外提交的文件
**文件**: `filename`
**问题描述**: 描述为什么这个文件不应该被提交
**修复方案**:
```bash
git rm filename
优先级: 高
### 2. Build Failure Risks (HIGH PRIORITY)
Check for configurations that reference files which may not exist:
**Pattern 1: extraResources referencing generated files**
```javascript
// electron-builder configs
'extraResources': [
{ 'from': 'path/to/generated/file', 'to': '...' }
]
If the file is generated by a script that may skip execution (e.g., platform-specific tools), the build will fail.
Fix Pattern:
- Create placeholder files when skipping generation
- Add file existence checks in CI
Pattern 2: Scripts with early exit without cleanup
if ! command -v tool &> /dev/null; then
echo "Tool not found"
exit 0
fi
if ! command -v tool &> /dev/null; then
echo "Tool not found"
mkdir -p "$OUTPUT_PATH"
touch "$OUTPUT_PATH/expected-file"
exit 0
fi
Report Format:
## 问题: 构建失败风险
**相关文件**:
- config-file.js
- script.sh
**问题描述**:
详细说明为什么构建可能失败
**修复方案**:
```bash
# 具体的代码修复
主要修改点:
- 修改点1
- 修改点2
优先级: 高
### 3. Script Accuracy (MEDIUM PRIORITY)
Check for inaccurate comments or misleading error messages:
```bash
# Check error messages and comments in shell scripts
grep -n "echo.*Warning\|echo.*Error\|#.*requires\|#.*only available" apps/*/scripts/*.sh
Common Issues:
- Claiming a tool is "only available on X" when it's available elsewhere
- Incorrect version requirements
- Misleading prerequisite descriptions
Report Format:
## 问题: 脚本注释不准确
**文件**: `path/to/script.sh:LINE`
**问题描述**:
注释说 X,但实际上 Y
**修复方案**:
将第 N 行从:
```bash
echo "原始内容"
改为:
echo "修正后内容"
优先级: 中
### 4. CI Workflow Verification (LOW-MEDIUM PRIORITY)
Check for missing verification steps in CI:
**Pattern: Operations without verification**
```yaml
# Bad: no verification after generation
- name: Generate Assets
run: bash scripts/generate.sh
# Good: verify generation succeeded
- name: Generate Assets
run: bash scripts/generate.sh
- name: Verify Assets
run: bash scripts/verify.sh
Check Points:
- File generation steps should have verification
- Platform-specific steps should handle cross-platform CI
- Critical paths should fail fast with clear errors
Report Format:
## 问题: CI 工作流缺少验证
**相关文件**:
- .github/workflows/workflow.yml
**问题描述**:
CI 在执行某操作后没有验证是否成功
**修复方案**:
```yaml
- name: Verify Step
run: |
# verification logic
优先级: 低
### 5. Documentation Consistency (LOW PRIORITY)
Check for PR description matching actual changes:
```bash
# List all changed files
git diff origin/x...HEAD --name-status
# Compare with PR description
gh pr view PRNUM --json body
Check Points:
- All significant file changes mentioned in PR
- iOS/Android changes called out for mobile icon updates
- Breaking changes clearly documented
- Migration steps provided if needed
Output Format
Summary Report Structure
# PR #NUMBER 代码审查建议
## 问题 1: [问题标题]
**文件**: `path/to/file`
**问题描述**: 详细描述问题
**修复方案**:
```code
具体修复代码
优先级: 高/中/低
问题 2: [问题标题]
...
修改清单总结
| 优先级 | 文件 | 修改类型 | 描述 |
|---|
| 高 | file1 | 修改 | 描述 |
| 中 | file2 | 删除 | 描述 |
测试验证
修复完成后,建议进行以下测试:
- 测试场景1
- 测试场景2
## Priority Definitions
| Priority | Description | Action |
|----------|-------------|--------|
| **高 (High)** | Build failures, security issues, data loss risks | Must fix before merge |
| **中 (Medium)** | Incorrect behavior, misleading docs, maintainability | Should fix before merge |
| **低 (Low)** | Nice-to-have improvements, minor inconsistencies | Can fix in follow-up PR |
## Quick Commands
```bash
# Get PR diff summary
git diff origin/x...HEAD --stat
# Check for generated file references in configs
grep -r "extraResources\|extraFiles" apps/*/electron-builder*.js
# Find shell scripts with early exits
grep -n "exit 0\|exit 1" apps/*/scripts/*.sh
# Check CI workflow steps
yq '.jobs.*.steps[].name' .github/workflows/*.yml 2>/dev/null || \
grep -A2 "name:" .github/workflows/*.yml
# Find useEffect with eslint-disable (potential dependency issues)
git diff origin/x...HEAD | grep -A5 "useEffect" | grep "eslint-disable"
# Find loops with await inside (performance issue)
git diff origin/x...HEAD | grep -E "for.*\{|forEach|\.map\(" -A10 | grep "await"
# Check for deprecated packages in new dependencies
git diff origin/x...HEAD -- '**/package.json' | grep '^\+' | \
grep -oE '"[^"]+": "[^"]+"' | cut -d'"' -f2 | \
xargs -I{} sh -c 'npm view {} deprecated 2>/dev/null && echo "^^^ {}"'
# Find silent error handling (catch without user feedback)
git diff origin/x...HEAD | grep -A3 "catch" | grep -v "Toast\|throw\|error"
# Check for missing file size validation in uploads
git diff origin/x...HEAD | grep -B5 -A10 "file\." | grep -v "size"
# Find potential null/undefined issues (missing optional chaining)
git diff origin/x...HEAD | grep -E "\.current\.[a-zA-Z]|ref\.[a-zA-Z]" | grep -v "?."
# Find array index access without bounds check
git diff origin/x...HEAD | grep -E "\[index\]|\[i\]|\[0\]" -A2 | grep -v "if.*length\|if.*!"
# Find potential race conditions (state updates in async)
git diff origin/x...HEAD | grep -B5 "setState\|set[A-Z]" | grep -E "then\(|await"
# Find platform-specific files
git diff origin/x...HEAD --name-only | grep -E "\.(android|ios|native|desktop)\.(ts|tsx)$"
# Find BigNumber operations without type coercion
git diff origin/x...HEAD | grep -E "BigNumber|shiftedBy|dividedBy" | grep -v "Number("
# Find debounced functions that may not return promises
git diff origin/x...HEAD | grep -E "debounce|setTimeout.*validate" -A5 | grep -v "Promise\|resolve"
# Find setState without clearing stale data
git diff origin/x...HEAD | grep -B3 -A3 "useEffect" | grep -E "fetch|load" | grep -v "set.*\[\]|set.*null"
# Find captured refs in useEffect cleanup (potential stale ref)
git diff origin/x...HEAD | grep -B10 "return.*=>" | grep "const.*=.*Ref.current"
# Check for division operations without zero guards
git diff origin/x...HEAD | grep -E "/ [a-zA-Z]" | grep -v "if.*===.*0\|if.*>.*0"
# Find map/forEach with index that mutates array
git diff origin/x...HEAD | grep -E "\.map\(|\.forEach\(" -A5 | grep -E "splice|shift|pop"
6. React Hooks Safety (HIGH PRIORITY)
Check for hooks-related issues that can cause infinite loops, memory leaks, or race conditions:
Pattern 1: useEffect with missing dependencies
useEffect(() => {
doSomething(someValue);
}, []);
useEffect(() => {
doSomething(someValue);
}, [someValue]);
Pattern 2: Non-functional setState in useCallback
const handleClick = useCallback(() => {
setState({ ...state, updated: true });
}, [state]);
const handleClick = useCallback(() => {
setState(prev => ({ ...prev, updated: true }));
}, []);
Pattern 3: Missing cleanup in useEffect
useEffect(() => {
const timer = setInterval(() => { ... }, 1000);
if (condition) return;
return () => clearInterval(timer);
}, []);
useEffect(() => {
if (condition) return () => {};
const timer = setInterval(() => { ... }, 1000);
return () => clearInterval(timer);
}, []);
Pattern 4: Async validation without abort
useEffect(() => {
validateAsync(value).then(setResult);
}, [value]);
useEffect(() => {
const controller = new AbortController();
validateAsync(value, controller.signal).then(setResult);
return () => controller.abort();
}, [value]);
Report Format:
## 问题: Hooks 安全风险
**文件**: `path/to/component.tsx:LINE`
**问题描述**:
- 风险类型:死循环/内存泄漏/竞态条件/过时闭包
- 具体说明
**修复方案**:
```typescript
// 修复后的代码
优先级: 高
### 7. Concurrent Request Control (HIGH PRIORITY)
Check for code that may overwhelm backend with too many requests:
**Pattern 1: Sequential await in loop (performance issue)**
```typescript
// Bad: 500 addresses = 500 sequential API calls = 50+ seconds
for (const address of addresses) {
await validateAddress(address); // O(n) API calls
}
// Good: concurrent with rate limiting
import pLimit from 'p-limit';
const limit = pLimit(10); // max 10 concurrent
await Promise.all(addresses.map(addr =>
limit(() => validateAddress(addr))
));
Pattern 2: Missing request deduplication
useEffect(() => {
const interval = setInterval(() => {
fetchData();
}, 1000);
return () => clearInterval(interval);
}, [fetchData]);
const isLoading = useRef(false);
useEffect(() => {
const interval = setInterval(async () => {
if (isLoading.current) return;
isLoading.current = true;
try { await fetchData(); }
finally { isLoading.current = false; }
}, 1000);
return () => clearInterval(interval);
}, []);
Pattern 3: Unbatched validation
items.forEach(async item => {
const result = await api.validate(item);
});
const results = await api.validateBatch(items);
Check Points:
- Loops with
await inside - consider Promise.all with p-limit
- Polling without request guards
- Form validation that calls API per field
- File processing without chunking
Report Format:
## 问题: 大量并发/串行请求
**文件**: `path/to/file.tsx:LINE`
**问题描述**:
| 场景 | 请求数 | 预估耗时 |
|------|--------|----------|
| 100 项 | 100 次 | 10 秒 |
| 500 项 | 500 次 | 50 秒 |
**修复方案**:
```typescript
// 使用并发控制
import pLimit from 'p-limit';
const limit = pLimit(10);
await Promise.all(items.map(i => limit(() => process(i))));
优先级: 高
### 8. Deprecated Dependencies (MEDIUM PRIORITY)
Check for deprecated or renamed packages:
```bash
# Check for deprecated packages in package.json changes
git diff origin/x...HEAD -- '**/package.json' | grep -E '^\+.*"[^"]+": "[^"]+"'
# Verify package status
npm view PACKAGE_NAME deprecated 2>/dev/null
Common Patterns:
- Package renamed (e.g.,
react-native-document-picker → @react-native-documents/picker)
- Package no longer maintained
- Security vulnerabilities in dependencies
Report Format:
## 问题: 依赖包已废弃
**文件**: `package.json`
**包名**: `deprecated-package`
**问题描述**:
该包已被废弃,官方推荐迁移到新包
**迁移建议**:
```bash
yarn remove deprecated-package
yarn add new-package
API 变更:
import { old } from 'deprecated-package';
import { new } from 'new-package';
优先级: 中
### 9. Error Handling Patterns (MEDIUM PRIORITY)
Check for proper error handling:
**Pattern 1: Silent failures**
```typescript
// Bad: error swallowed, user gets no feedback
try {
await submitForm();
} catch (error) {
console.error(error); // Only logged, not shown
}
// Good: show error to user
try {
await submitForm();
} catch (error) {
Toast.error({ title: error.message });
}
Pattern 2: Empty catch blocks
try { riskyOperation(); } catch {}
try { riskyOperation(); } catch (e) { console.warn(e); }
Pattern 3: Silent early returns
const handleSubmit = () => {
if (!data) return;
};
const handleSubmit = () => {
if (!data) {
Toast.warning({ title: 'Please fill required fields' });
return;
}
};
Report Format:
## 问题: 错误处理不完整
**文件**: `path/to/file.tsx:LINE`
**问题描述**:
错误被静默处理,用户无法得知失败原因
**修复方案**:
```typescript
// 添加用户反馈
Toast.error({ title: error.message });
优先级: 中
### 10. Null/Undefined Safety (HIGH PRIORITY)
Check for missing null/undefined guards that can cause crashes:
**Pattern 1: Optional chaining missing**
```typescript
// Bad: crashes if ref is undefined
const value = ref.current.getValue();
// Good: optional chaining
const value = ref.current?.getValue();
Pattern 2: Array access without bounds check
const item = items[index];
item.name;
const item = items[index];
if (!item) return;
item.name;
Pattern 3: Callback without null check
onDomReady(() => {
webviewRef.current.reload();
});
onDomReady(() => {
if (!webviewRef.current) return;
webviewRef.current.reload();
});
Pattern 4: Index shifting after array mutation
for (let i = 0; i < items.length; i++) {
if (shouldRemove(items[i])) {
items.splice(i, 1);
}
}
const filtered = items.filter(item => !shouldRemove(item));
Common Crash Sources (from Sentry):
TypeError: Cannot read property 'X' of undefined
nil NSString crash in TurboModule
EXC_BAD_ACCESS when URI is nil
RetryableMountingLayerException: Unable to find viewState for tag
Report Format:
## 问题: 空值安全
**文件**: `path/to/file.tsx:LINE`
**问题描述**:
缺少空值检查,可能导致 TypeError 或崩溃
**修复方案**:
```typescript
// 添加可选链或空值检查
if (!value) return;
优先级: 高
### 11. Race Conditions & Cleanup (HIGH PRIORITY)
Check for race conditions in React components, especially with React Native Fabric:
**Pattern 1: State update after unmount**
```typescript
// Bad: may update unmounted component
useEffect(() => {
fetchData().then(data => {
setState(data); // Component may be unmounted
});
}, []);
// Good: with mount check
useEffect(() => {
let isMounted = true;
fetchData().then(data => {
if (isMounted) setState(data);
});
return () => { isMounted = false; };
}, []);
Pattern 2: Dialog close + navigation race
const handleConfirm = () => {
dialog.close();
navigation.push('NextPage');
};
const handleConfirm = async () => {
dialog.close();
await new Promise(r => setTimeout(r, 100));
navigation.push('NextPage');
};
Pattern 3: WebView ref access after cleanup
useEffect(() => {
return () => {
webviewRef.current?.stopLoading();
};
}, []);
useEffect(() => {
const webview = webviewRef.current;
return () => {
webview?.stopLoading();
};
}, []);
Common Fabric Crashes:
RetryableMountingLayerException - View already unmounted
SurfaceControl crashes on Android - MIUI/HyperOS specific
SIGSEGV during navigation cleanup - WebView race condition
Report Format:
## 问题: 竞态条件
**文件**: `path/to/file.tsx:LINE`
**问题描述**:
组件卸载时状态更新/Dialog 关闭与导航竞争
**修复方案**:
```typescript
// 添加 isMounted 检查或延迟
优先级: 高
### 12. Platform-specific Issues (MEDIUM PRIORITY)
Check for platform-specific code that may cause issues:
**Android Common Issues:**
```typescript
// MIUI/HyperOS splash screen crash
// - SurfaceControl error needs defensive handling
// - Add try-catch for splash operations
// Layout re-measurement
// - tabBarHidden changes need layout update
// - Use LayoutAnimation or forceUpdate
// OOM with images
// - Check bitmap memory limits
// - Use resizeMode and memory-efficient loading
iOS Common Issues:
if (!uri || uri.length === 0) return null;
Quick Check Commands:
git diff origin/x...HEAD --name-only | grep -E "\.(android|ios)\.(ts|tsx)$"
git diff origin/x...HEAD | grep -E "NativeModules|TurboModule|requireNativeComponent"
Report Format:
## 问题: 平台特定问题
**平台**: Android/iOS
**文件**: `path/to/file.tsx:LINE`
**问题描述**:
[平台] 特有的崩溃或异常行为
**修复方案**:
```typescript
// 平台特定的防御性代码
优先级: 中
### 13. Type Safety for Numeric Operations (MEDIUM PRIORITY)
Check for type coercion issues with BigNumber/decimal operations:
**Pattern 1: Ensure number type before BigNumber**
```typescript
// Bad: decimals might be string from JSON
const amount = new BigNumber(value).shiftedBy(-decimals);
// Good: ensure number type
const amount = new BigNumber(value).shiftedBy(-Number(decimals));
Pattern 2: String vs Number in API responses
const balance = response.balance;
const balance = String(response.balance);
Pattern 3: Division with potential zero
const rate = amount / total;
const rate = total > 0 ? amount / total : 0;
Report Format:
## 问题: 数值类型安全
**文件**: `path/to/file.tsx:LINE`
**问题描述**:
类型强制转换可能导致 NaN 或计算错误
**修复方案**:
```typescript
// 确保类型正确
const value = Number(input);
if (isNaN(value)) throw new Error('Invalid number');
优先级: 中
### 14. Stale Data Management (MEDIUM PRIORITY)
Check for stale data issues when context changes:
**Pattern 1: Clear cache on context switch**
```typescript
// Bad: stale provider list shown when switching
const [providers, setProviders] = useState([]);
useEffect(() => {
fetchProviders(type).then(setProviders);
}, [type]);
// Good: clear before fetching
useEffect(() => {
setProviders([]); // Clear stale data immediately
fetchProviders(type).then(setProviders);
}, [type]);
Pattern 2: State/callback captured at wrong time
useEffect(() => {
const savedCallback = onUpdate;
const interval = setInterval(() => {
savedCallback(getData());
}, 1000);
return () => clearInterval(interval);
}, []);
const onUpdateRef = useRef(onUpdate);
onUpdateRef.current = onUpdate;
useEffect(() => {
const interval = setInterval(() => {
onUpdateRef.current(getData());
}, 1000);
return () => clearInterval(interval);
}, []);
Note on Refs in Cleanup: For DOM/component refs (like webviewRef), you SHOULD capture the ref before cleanup to ensure you're cleaning up the correct resource. See Section 11, Pattern 3 for the correct ref cleanup pattern.
Pattern 3: State derived from props not updating
const [value, setValue] = useState(props.initialValue);
const [value, setValue] = useState(props.initialValue);
useEffect(() => {
setValue(props.initialValue);
}, [props.initialValue]);
Report Format:
## 问题: 陈旧数据
**文件**: `path/to/file.tsx:LINE`
**问题描述**:
上下文切换时显示旧数据,造成用户困惑
**修复方案**:
```typescript
// 切换时立即清除旧数据
setData(null);
fetchNewData().then(setData);
优先级: 中
### 15. Debounced Async Validation (MEDIUM PRIORITY)
Check for issues with debounced validation that returns promises:
**Pattern 1: Promise never resolves**
```typescript
// Bad: debounced function may not resolve promise
const debouncedValidate = useMemo(() => {
let timeoutId: NodeJS.Timeout;
return (value: string) => {
clearTimeout(timeoutId);
timeoutId = setTimeout(async () => {
await validate(value); // Promise not returned!
}, 300);
};
}, []);
// Good: track and resolve promises
const debouncedValidate = useCallback((value: string) => {
return new Promise<boolean>((resolve) => {
clearTimeout(timeoutRef.current);
timeoutRef.current = setTimeout(async () => {
const result = await validate(value);
resolve(result);
}, 300);
});
}, [validate]);
Pattern 2: Form validation hangs
rules={{
validate: (value) => {
debouncedValidate(value);
return true;
}
}}
rules={{
validate: (value) => debouncedValidate(value)
}}
Pattern 3: Cleanup pending validations
useEffect(() => {
return () => {
};
}, []);
useEffect(() => {
return () => {
if (timeoutRef.current) clearTimeout(timeoutRef.current);
if (pendingResolve.current) pendingResolve.current(true);
};
}, []);
Report Format:
## 问题: 防抖验证 Promise 处理
**文件**: `path/to/file.tsx:LINE`
**问题描述**:
防抖验证函数未正确返回 Promise,导致表单验证挂起
**修复方案**:
```typescript
// 确保返回 Promise 并在卸载时清理
优先级: 中
### 16. Security Considerations for Bulk Operations (HIGH PRIORITY)
Check security aspects of features handling multiple items:
**Pattern 1: File upload without size limits**
```typescript
// Bad: no file size check
const handleFile = async (file: File) => {
const content = await file.text(); // Could be gigabytes
};
// Good: check size first
const MAX_SIZE = 5 * 1024 * 1024; // 5MB
const handleFile = async (file: File) => {
if (file.size > MAX_SIZE) {
throw new Error('File too large');
}
const content = await file.text();
};
Pattern 2: Hardcoded contract addresses
const CONTRACT = '0x123...';
import { getAddress } from 'ethers';
const CONTRACT = getAddress('0x123...');
Pattern 3: Missing input validation
const amounts = userInput.split(',').map(Number);
const amounts = userInput.split(',').map(v => {
const num = Number(v.trim());
if (isNaN(num) || num < 0) throw new Error('Invalid amount');
return num;
});
Report Format:
## 问题: 安全风险
**文件**: `path/to/file.tsx:LINE`
**风险类型**: 输入验证/资金安全/DoS风险
**问题描述**:
详细说明安全风险
**修复方案**:
```typescript
// 安全的实现
优先级: 高
## Review Workflow
1. **Scope**: Run `git diff origin/x...HEAD --stat` to understand change scope
2. **Files**: Check for accidental commits and generated file references
3. **Scripts**: Review shell scripts for proper error handling and placeholders
4. **CI**: Verify CI workflows have appropriate verification steps
5. **Hooks**: Check React hooks for dependency issues, memory leaks, infinite loops
6. **Requests**: Verify concurrent/sequential request handling is optimized
7. **Dependencies**: Check for deprecated packages in new dependencies
8. **Errors**: Ensure proper error handling with user feedback
9. **Null Safety**: Check for missing null/undefined guards
10. **Race Conditions**: Look for Fabric race conditions, cleanup issues
11. **Platform**: Check Android/iOS specific crash patterns
12. **Type Safety**: Verify numeric types before BigNumber/decimal operations
13. **Stale Data**: Check for cache clearing on context switches
14. **Debounce**: Verify debounced async functions return proper promises
15. **Security**: Review security aspects for bulk/batch operations
16. **Docs**: Ensure PR description matches actual changes
17. **Report**: Generate structured report with priorities and fix suggestions