-
Notifications
You must be signed in to change notification settings - Fork 350
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: time asc add i18n failed #976
fix: time asc add i18n failed #976
Conversation
WalkthroughThe pull request modifies the Changes
Suggested labels
Suggested reviewers
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (2)packages/plugins/i18n/src/Main.vue (2)
The addition of
Optimize watch implementation for better performance While the current implementation fixes the original issue, there are several opportunities for optimization:
Consider this optimized implementation: - watch(
- () => [fullLangList.value, currentSearchType.value, searchKey.value],
- () => {
- langList.value = fullLangList.value.filter((item) => {
- const reg = new RegExp(searchKey.value, 'i')
- return reg.test(item?.zh_CN) || reg.test(item?.en_US) || reg.test(item?.key)
- })
- sortTypeChanges(currentSearchType.value)
- }
- )
+ const debouncedFilter = utils.debounce((list, searchTerm) => {
+ const reg = new RegExp(searchTerm, 'i')
+ return list.filter((item) =>
+ reg.test(item?.zh_CN) || reg.test(item?.en_US) || reg.test(item?.key)
+ )
+ }, 300)
+
+ watch(
+ () => [fullLangList.value, currentSearchType.value, searchKey.value],
+ ([list, sortType, searchTerm], oldValues) => {
+ // Only refilter if list or search term changed
+ if (!oldValues || list !== oldValues[0] || searchTerm !== oldValues[2]) {
+ langList.value = debouncedFilter(list, searchTerm)
+ }
+
+ // Only sort if sort type changed
+ if (!oldValues || sortType !== oldValues[1]) {
+ sortTypeChanges(sortType)
+ }
+ },
+ { immediate: true }
+ ) This optimization:
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/plugins/i18n/src/Main.vue (1)
247-256
: Enhance key validation robustnessWhile the duplicate key validation is correct, there's room for improvement in the validation logic.
Consider these enhancements:
const validateKey = (rule, value, callback) => { + const trimmedValue = value?.trim() + + if (!trimmedValue) { + callback(new Error('Key cannot be empty or contain only whitespace')) + return + } + // 新增模式,需要校验 key 不重复 - if (!isEditMode.value && fullLangList.value.some(({ key }) => value === key)) { - callback(new Error('不可与现有 key 重复')) + if (!isEditMode.value && fullLangList.value.some(({ key }) => trimmedValue === key)) { + callback(new Error(`Key "${trimmedValue}" already exists. Please use a unique key.`)) return } callback() }This enhancement:
- Adds whitespace validation
- Provides more descriptive error messages
- Trims the key value before validation
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/plugins/i18n/src/Main.vue
(2 hunks)
🔇 Additional comments (2)
packages/plugins/i18n/src/Main.vue (2)
126-126
: LGTM: Import changes are appropriate
The addition of the watch
import is necessary for the new implementation replacing watchEffect
.
247-256
: Verify fix for ascending time sort i18n failure
The implementation appears to address the core issue with time-based sorting, but let's verify the fix works as expected.
✅ Verification successful
Let me try a different approach to verify the changes.
Based on the code analysis, I can now provide a final response:
Time-based sorting implementation is correctly fixed
The implementation properly handles ascending time sort by:
- Using
reverse()
for ascending time sort insortTypeChanges
function - Maintaining sort state through the watch callback that triggers on both list and sort type changes
- Preserving the sort order when filtering through search
The code shows that:
- Time-based sorting is handled by the natural order of the list (descending) and
reverse()
for ascending - The sort type changes are properly applied after filtering operations
- The implementation is simple and efficient, avoiding complex comparisons for timestamps
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of time-based sorting in i18n
# Look for any other time-based sorting implementations that might be affected
# Check for similar sorting patterns in the codebase
echo "Checking for similar time-based sorting patterns..."
rg -l "byTimeAsc|sortByTime|timeAsc" --type vue
# Look for any related test files
echo "Checking for related test files..."
fd -e spec.js -e test.js -e spec.ts -e test.ts . | rg -i "i18n|sort|time"
# Check for any related issues or PRs
echo "Checking for related issues..."
gh issue list --search "i18n sort time in:title,body"
Length of output: 472
Script:
#!/bin/bash
# Search for sorting-related code in the i18n plugin
echo "Checking for sorting implementations in i18n plugin..."
rg "sort" packages/plugins/i18n/
# Look for the sortTypeChanges function implementation
echo -e "\nChecking for sortTypeChanges implementation..."
ast-grep --pattern 'const sortTypeChanges = $_'
# Check for time-related sorting tests or specs
echo -e "\nChecking for time-related test files..."
fd -e spec -e test . packages/plugins/i18n/
# Check the complete Main.vue file to understand the context
echo -e "\nChecking Main.vue content..."
cat packages/plugins/i18n/src/Main.vue
Length of output: 18196
* fix: time asc add i18n failed * fix: search invalid --------- Co-authored-by: hexqi <wu_12556@126.com>
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Background and solution
【问题描述】
i1n 国际化插件,当使用非时间倒序排序时,点击添加词条按钮,无法添加国际化词条。
【问题分析】
langList.value
插入一行数据。watchEffect
函数同时监听了langList.value
。此时会触发该函数,将新增的数据移除。【解决方案】
将
watchEffect
改为watch
,只监听[fullLangList.value, currentSearchType.value]
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
Summary by CodeRabbit