-
Notifications
You must be signed in to change notification settings - Fork 0
fix: remove hardcoded 48-character limit from text inputs #18
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
base: devin_pr_code_review_bench_100_devin2_base_fix_remove_hardcoded_48-character_limit_from_text_inputs_pr417
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,7 +26,7 @@ import DifyLogo from '@/app/components/base/logo/dify-logo' | |
| import Toast from '@/app/components/base/toast' | ||
| import Res from '@/app/components/share/text-generation/result' | ||
| import RunOnce from '@/app/components/share/text-generation/run-once' | ||
| import { appDefaultIconBackground, BATCH_CONCURRENCY, DEFAULT_VALUE_MAX_LEN } from '@/config' | ||
| import { appDefaultIconBackground, BATCH_CONCURRENCY } from '@/config' | ||
| import { useGlobalPublicStore } from '@/context/global-public-context' | ||
| import { useWebAppStore } from '@/context/web-app-context' | ||
| import { useAppFavicon } from '@/hooks/use-app-favicon' | ||
|
|
@@ -196,6 +196,7 @@ const TextGeneration: FC<IMainProps> = ({ | |
| return false | ||
| } | ||
| const headerData = data[0] | ||
| console.log('Checking batch inputs:', { dataLength: data.length, headerData }) | ||
| let isMapVarName = true | ||
| promptConfig?.prompt_variables.forEach((item, index) => { | ||
| if (!isMapVarName) | ||
|
|
@@ -257,10 +258,9 @@ const TextGeneration: FC<IMainProps> = ({ | |
| if (errorRowIndex !== 0) | ||
| return | ||
| 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) { | ||
| moreThanMaxLengthVarName = varItem.name | ||
| maxLength = maxLen | ||
| maxLength = varItem.max_length | ||
| errorRowIndex = index + 1 | ||
| return | ||
| } | ||
|
Comment on lines
+261
to
266
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Batch validation bypassed when max_length is undefined - comparison with undefined always returns false When Click to expandRoot CauseThe PR removes the fallback to if (item[varIndex].length > varItem.max_length)will always evaluate to 100 > undefined // false
0 > undefined // falseImpactFor batch operations on text generation apps, if a prompt variable doesn't have a
Expected vs Actual
Recommendation: Add a guard condition to skip the max_length check when it's undefined: Was this helpful? React with 👍 or 👎 to provide feedback. |
||
|
|
||
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.
🟡 Debug console.log statement left in production code
A
console.logstatement was added to the batch input validation function, which violates coding standards and should not be in production code.Click to expand
Location
web/app/components/share/text-generation/index.tsx:199Impact
Recommendation: Remove the console.log statement or replace with proper logging mechanism.
Was this helpful? React with 👍 or 👎 to provide feedback.