Code Review Bench PR #30156 - fix: remove hardcoded 48-character limit from text inputs #5
Conversation
![gitar-bot-staging[bot]](https://avatars.githubusercontent.com/in/812680?s=60&v=4){kind=small}
| return false | ||
| } | ||
| const headerData = data[0] | ||
| console.log('Checking batch inputs:', { dataLength: data.length, headerData }) |
There was a problem hiding this comment.
⚠️ Quality: Debug console.log left in production code
A console.log('Checking batch inputs:', { dataLength: data.length, headerData }) statement has been added at line 199 inside the checkBatchInputs function. This appears to be a debugging statement accidentally left in the PR. It will log potentially sensitive user-uploaded batch data (header row contents) to the browser console in production, which is a data exposure concern and adds noise to the console output.
Was this helpful? React with 👍 / 👎
| console.log('Checking batch inputs:', { dataLength: data.length, headerData }) | |
| const headerData = data[0] | |
| let isMapVarName = true |
- Apply suggested fix
| {type === 'number' && ( | ||
| <Input | ||
| type="number" | ||
| type="text" |
There was a problem hiding this comment.
⚠️ Bug: Number input type changed from "number" to "text" — unrelated to PR
In prompt-value-panel/index.tsx at line 167, the input type for number-typed variables was changed from "number" to "text". This is unrelated to removing the 48-character limit and changes the behavior of number inputs:
type="number"provides browser-native numeric validation, a numeric keyboard on mobile, and spinner controlstype="text"accepts any text input with no numeric constraints
This appears to be an accidental change. If number inputs accept arbitrary text, downstream logic that expects numeric values will receive strings, potentially causing bugs in prompt variable handling.
Was this helpful? React with 👍 / 👎
| type="text" | |
| type="number" |
- Apply suggested fix
| 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 | ||
| } |
There was a problem hiding this comment.
⚠️ Edge Case: Batch validation relies on > undefined being false (NaN)
In the batch input validation at line 261, when varItem.max_length is undefined (no limit configured), the comparison item[varIndex].length > varItem.max_length relies on JavaScript's NaN comparison semantics (number > undefined → false). While this happens to produce the correct result (skip validation), it's fragile and unclear:
- It makes the code intent ambiguous — a reader can't tell if this is intentional or a bug
- If
max_lengthwere ever0, the comparison would erroneously flag all non-empty inputs
The PR context itself describes the intended pattern as if (varItem.type === 'string' && varItem.max_length), which explicitly guards against undefined. This guard should be added to match the stated design intent.
Was this helpful? React with 👍 / 👎
| 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 | |
| } | |
| if (varItem.type === 'string' && varItem.max_length) { | |
| if (item[varIndex].length > varItem.max_length) { |
- Apply suggested fix
There was a problem hiding this comment.
Configure merge blocking · Maintainers can dismiss this review. Gitar never approves changes.
Code Review
|
| Auto-apply | Compact |
|
|
Was this helpful? React with 👍 / 👎 | Gitar
2 participants
Benchmark PR for Gitar evaluation
Original PR: agentic-review-benchmarks#5
fix: remove hardcoded 48-character limit from text inputs