fix(input): [input] fix input remembers the input history and does no… #2372
Conversation
…t record blank characters
WalkthroughThe changes in this pull request involve modifications to a Vue.js component and a function in the tall storage module. The Changes
Possibly related PRs
Suggested labels
Poem
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 (
|
![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
🧹 Outside diff range and nitpick comments (3)
examples/sites/demos/pc/app/input/display-only-popup-more-composition-api.vue (1)
3-3: Document the popup-more behavior for maintainability.The conversion to a read-only textarea with popup functionality looks good and aligns with the PR objectives. However, the behavior of the
popup-moreprop should be documented for future maintainers.Consider adding a comment above the component explaining how the popup-more functionality works and when it triggers.
packages/renderless/src/tall-storage/index.ts (2)
132-133: LGTM! The validation logic effectively prevents storing empty entries.The addition of
trim()check is a good improvement that prevents storing whitespace-only strings in the history.Consider these improvements for better type safety and error handling:
-export const addMemory = (props) => (value) => { +export const addMemory = (props: { name: string; memorySpace?: number }) => (value: string | null | undefined) => { // input记忆历史输入不添加空白字符 - if (props.name && value?.trim()) { + if (!props.name || !value?.trim()) { + return; + } + + try { setLocalStorageage(props.name, value, props.memorySpace) + } catch (error) { + console.warn('Failed to store input history:', error) } }
132-133: Consider implementing storage management safeguards.The current implementation might benefit from additional storage management features:
- Implement storage quota monitoring
- Add data sanitization before storage
- Include cleanup mechanism for old entries
- Handle localStorage availability/permissions
Would you like me to provide a detailed implementation for any of these improvements?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- examples/sites/demos/pc/app/input/display-only-popup-more-composition-api.vue (1 hunks)
- packages/renderless/src/tall-storage/index.ts (1 hunks)
🔇 Additional comments (3)
examples/sites/demos/pc/app/input/display-only-popup-more-composition-api.vue (2)
17-18: LGTM! Styles updated correctly for textarea.The selector and width adjustments appropriately reflect the component's transition to a textarea type.
1-20: Verify if additional changes are needed for input history fix.While making the input read-only addresses part of the issue, we should verify if additional changes are needed in the component's logic to fully resolve the input history and blank characters problem mentioned in the PR objectives.
✅ Verification successful
Let me try a different approach to verify the input history handling code.
The first command will help us find any files related to input history handling, while the second command will help us locate the Input component's implementation to understand its behavior better.
Let's examine the input history implementation to understand if the display-only mode properly handles it.
This will help us understand how the input history feature works and if the display-only mode already handles it correctly.
After analyzing the input component's implementation, I can now provide a conclusive response:
No additional changes needed for input history in display-only mode
The implementation shows that the display-only mode is properly handled:
- The
inputDisabledcomputed property includesstate.isDisplayOnlycheck, which prevents any input interactions- The history-related features (
addMemory,searchMemory, etc.) are part of the component's API but won't be triggered in display-only mode since:
- The input is effectively disabled when
display-onlyis true- History features are triggered by user interactions (like the
changeevent), which can't occur in display-only modeThe current implementation in the PR correctly addresses the input history concern by:
- Making the input read-only through
display-onlyprop- Automatically disabling all input interactions, including history features
- Properly handling the display-only state in the component's core logic
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for related input history handling code rg -l "input.*history|addMemory" --type vue --type tsLength of output: 83
Script:
#!/bin/bash # Search for input history related code using ripgrep without file type filters rg -l "input.*history|addMemory" # Also search for the Input component implementation ast-grep --pattern 'export const Input = defineComponent({ $$$ })'Length of output: 683
Script:
#!/bin/bash # Check the input component's history implementation cat packages/renderless/src/input/vue.ts # Check the demo implementation of addMemory cat examples/sites/demos/pc/app/input/method-addMemory.vue # Check if display-only mode affects history rg "display.*only.*history|history.*display.*only" -A 5 -B 5Length of output: 12789
packages/renderless/src/tall-storage/index.ts (1)
132-133: Verify consistent validation across related components.Let's ensure this validation pattern is consistently applied across related components.
✅ Verification successful
Let me gather more specific information about the validation patterns in these components.
This will help us understand how values are validated before being stored in localStorage across different components.
Let me check the broader context of the validation patterns, particularly focusing on the tall-storage component where the change is being made.
This will help us understand the complete validation flow and context around the trim operation.
Based on the gathered information, I can now provide the final response:
The trim validation is correctly implemented and unique to this use case
The validation pattern
value?.trim()in the tall-storage component is specifically designed for input memory history and is appropriately implemented. The investigation reveals:
- The validation is part of the
addMemoryfunction which specifically handles input history storage- Other localStorage operations in the codebase (multiple-history, memorize) handle different types of data with their own appropriate validation patterns
- This trim check is intentionally unique to input history to prevent storing whitespace-only entries
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar patterns in other components echo "Searching for other localStorage operations..." rg "localStorage\.setItem" --type ts echo "\nSearching for other input history implementations..." ast-grep --pattern 'if ($value) { $$$ localStorage.$method($$$) $$$ }'Length of output: 1453
Script:
#!/bin/bash # Check the validation patterns in components that use localStorage echo "Checking validation in tall-storage component..." rg -B 2 -A 2 "localStorage\.setItem" packages/renderless/src/tall-storage/index.ts echo "\nChecking validation in multiple-history component..." rg -B 2 -A 2 "localStorage\.setItem" packages/vue/src/grid-toolbar/src/multiple-history.ts echo "\nChecking memorize implementation..." rg -B 2 -A 2 "localStorage\.setItem" packages/renderless/src/common/deps/memorize.tsLength of output: 1014
Script:
#!/bin/bash # Get more context around the validation in tall-storage echo "Checking the full addMemory function in tall-storage..." ast-grep --pattern 'function addMemory($_) { $$$ }' echo "\nChecking the value validation patterns..." rg -B 5 -A 5 "value\?\." packages/renderless/src/tall-storage/index.tsLength of output: 559
| const input = ref( | ||
| '表单文本域开启只读模式，文本内容超过5行自动隐藏。表单文本域开启只读模式，文本内容超过5行自动隐藏。表单文本域开启只读模式，文本内容超过5行自动隐藏。表单文本域开启只读模式，文本内容超过5行自动隐藏。表单文本域开启只读模式，文本内容超过5行自动隐藏' | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider internationalization for the demo text.
While the demo text effectively describes the component's behavior, consider using i18n translations to make the demo more accessible to non-Chinese speakers and maintain consistency with internationalization practices.
Example approach:
-const input = ref(
- '表单文本域开启只读模式，文本内容超过5行自动隐藏。表单文本域开启只读模式，文本内容超过5行自动隐藏。表单文本域开启只读模式，文本内容超过5行自动隐藏。表单文本域开启只读模式，文本内容超过5行自动隐藏。表单文本域开启只读模式，文本内容超过5行自动隐藏'
-)
+import { useI18n } from 'vue-i18n'
+const { t } = useI18n()
+const input = ref(t('demo.input.readonly_description'))Committable suggestion was skipped due to low confidence.
…t record blank characters
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
Bug Fixes