fix: remove hardcoded 48-character limit from text inputs#5
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request removes the hardcoded 48-character limit (DEFAULT_VALUE_MAX_LEN) from text inputs across the application, making max_length an optional property that can be omitted or explicitly set on a per-field basis.
Changes:
- Removed max_length defaults from variable templates (VAR_ITEM_TEMPLATE, VAR_ITEM_TEMPLATE_IN_WORKFLOW, VAR_ITEM_TEMPLATE_IN_PIPELINE)
- Updated input components to use max_length directly without fallback to DEFAULT_VALUE_MAX_LEN
- Removed logic that automatically set max_length when creating certain input types
- Added test coverage for maxLength behavior when undefined vs explicitly set
- Changed number input type from "number" to "text" in one component to support maxLength attribute
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| web/config/index.ts | Removed max_length property from three variable template objects |
| web/utils/var.ts | Removed destructuring exclusion of max_length in workflow variable creation |
| web/app/components/share/text-generation/run-once/index.tsx | Removed DEFAULT_VALUE_MAX_LEN import and fallback in input maxLength |
| web/app/components/share/text-generation/index.tsx | Removed DEFAULT_VALUE_MAX_LEN import and fallback in batch validation; added console.log |
| web/app/components/app/configuration/prompt-value-panel/index.tsx | Removed DEFAULT_VALUE_MAX_LEN import and fallback; changed number input type to "text" |
| web/app/components/app/configuration/debug/chat-user-input.tsx | Removed DEFAULT_VALUE_MAX_LEN import and fallback; added maxLength to paragraph textarea |
| web/app/components/app/configuration/config-var/index.tsx | Removed auto-setting max_length for textInput type |
| web/app/components/app/configuration/config-var/config-modal/index.tsx | Removed auto-setting max_length for paragraph type |
| web/app/components/rag-pipeline/components/panel/input-field/editor/form/hooks.ts | Removed auto-setting max_length for paragraph type |
| web/app/components/share/text-generation/run-once/index.spec.tsx | Added tests verifying maxLength behavior with and without max_length set |
| web/app/components/rag-pipeline/components/panel/input-field/editor/form/index.spec.tsx | Removed obsolete test for auto-setting max_length on paragraph type change |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return false | ||
| } | ||
| const headerData = data[0] | ||
| console.log('Checking batch inputs:', { dataLength: data.length, headerData }) |
There was a problem hiding this comment.
This console.log statement appears to be debugging code that was left in. It should be removed before merging to production.
| console.log('Checking batch inputs:', { dataLength: data.length, headerData }) |
| if (varItem.type === 'string') { | ||
| const maxLen = varItem.max_length || DEFAULT_VALUE_MAX_LEN | ||
| if (item[varIndex].length > maxLen) { | ||
| if (item[varIndex].length > varItem.max_length) { |
There was a problem hiding this comment.
This code has a critical bug when max_length is undefined. The comparison item[varIndex].length > varItem.max_length will evaluate to false when max_length is undefined (since any number > undefined is false), but it should skip the check entirely when max_length is not set. This should be changed to check if max_length exists before comparing: if (varItem.max_length && item[varIndex].length > varItem.max_length).
| if (item[varIndex].length > varItem.max_length) { | |
| if (typeof varItem.max_length === 'number' && item[varIndex].length > varItem.max_length) { |
| type="number" | ||
| type="text" | ||
| value={inputs[key] ? `${inputs[key]}` : ''} | ||
| onChange={(e) => { handleInputValueChange(key, e.target.value) }} |
There was a problem hiding this comment.
The input type was changed from "number" to "text", which removes browser-native numeric validation and allows non-numeric input. This change appears to be intentional (to allow maxLength on number inputs), but may allow users to enter invalid values like "abc" for a number field. Consider adding explicit validation to ensure only numeric values are accepted, or document why this change was made.
| onChange={(e) => { handleInputValueChange(key, e.target.value) }} | |
| onChange={(e) => { | |
| const value = e.target.value | |
| // Allow only digits (or empty string) for numeric inputs | |
| if (/^\d*$/.test(value)) { | |
| handleInputValueChange(key, value) | |
| } | |
| }} |
Benchmark PR from agentic-review-benchmarks#5