evals UI: Include comments in grouped question CSV#68
Merged
Conversation
📝 WalkthroughWalkthroughUpdates CSV export generation in the evaluations page to add per-score comment columns and stronger CSV cell handling: values are escaped (quotes/newlines), optionally prefixed to prevent CSV-injection, and both grouped (horizontal) and row (individual) formats emit two quoted headers per score (value + comment). Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip You can disable sequence diagrams in the walkthrough.Disable the |
nishika26
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/evaluations/`[id]/page.tsx:
- Line 240: The CSV export currently writes raw comment text via the expression
score?.comment inside the row.push(...) call, which allows CSV/formula injection
if a comment begins with =, +, -, or @; sanitize comments before CSVizing by
first escaping internal quotes/newlines as already done, then detect leading
characters (=, +, -, @) and prefix the cell content with a safe neutral
character (e.g. a single quote or a space) to neutralize spreadsheet formula
evaluation; apply this same sanitization wherever score?.comment is exported
(including the other occurrence with row.push(...)). Ensure you still wrap
fields in quotes after this sanitization.
- Line 215: The header concatenation injects raw score `name` values into
`csvContent`, which breaks CSV structure when names contain commas, quotes, or
newlines; sanitize `name` before interpolation by creating/using a small helper
(e.g., escapeCsv) that replaces internal double quotes with two double quotes
and wraps the field in double quotes (to cover commas/newlines) and then use
that sanitized value when building the header (the line where csvContent is
appended with `,${name} (${i}),${name} (${i}) - comment`); update any other
places that write `name` into CSV cells to use the same escapeCsv helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3c153828-ecc3-4d54-9c51-7c304bc501f5
📒 Files selected for processing (1)
app/evaluations/[id]/page.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/evaluations/[id]/page.tsx (1)
238-250: CSV injection prevention is correctly implemented for grouped export comments.The implementation properly defends against spreadsheet formula injection by prefixing dangerous characters. The quote escaping and column alignment for missing comments are handled correctly.
For consistency, consider applying the same injection prevention to the row export at line 315, which currently lacks this safeguard:
score?.comment ? `"${score.comment.replace(/"/g, '""').replace(/\n/g, ' ')}"` : '',
Suggested refactor for row export consistency (line 315)
...scoreNames.flatMap(name => { const score = item.trace_scores?.find(s => s.name === name); + let commentValue = ''; + if (score?.comment) { + let sanitized = score.comment.replace(/"/g, '""').replace(/\n/g, ' '); + if (/^[=+\-@]/.test(sanitized)) { + sanitized = ' ' + sanitized; + } + commentValue = `"${sanitized}"`; + } return [ score ? score.value : 'N/A', - score?.comment ? `"${score.comment.replace(/"/g, '""').replace(/\n/g, ' ')}"` : '' + commentValue ]; })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/evaluations/`[id]/page.tsx around lines 238 - 250, The row export at the score rendering (`score?.comment ? \`"\${score.comment.replace(/"/g, '""').replace(/\n/g, ' ')}"\` : ''`) lacks CSV-injection protection; update that expression to mirror the grouped export logic used in the scoreNames.forEach block: first escape double-quotes and newlines, then prefix a space if the sanitized string begins with =, +, -, or @, and finally wrap in quotes (or emit empty string when missing). Locate the inline ternary that references score?.comment and replace it with this sanitized flow so both grouped and row exports use the same CSV-injection-safe routine.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/evaluations/`[id]/page.tsx:
- Around line 238-250: The row export at the score rendering (`score?.comment ?
\`"\${score.comment.replace(/"/g, '""').replace(/\n/g, ' ')}"\` : ''`) lacks
CSV-injection protection; update that expression to mirror the grouped export
logic used in the scoreNames.forEach block: first escape double-quotes and
newlines, then prefix a space if the sanitized string begins with =, +, -, or @,
and finally wrap in quotes (or emit empty string when missing). Locate the
inline ternary that references score?.comment and replace it with this sanitized
flow so both grouped and row exports use the same CSV-injection-safe routine.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dbb51fdf-db60-49e2-a584-3a6f74705bf0
📒 Files selected for processing (1)
app/evaluations/[id]/page.tsx
vprashrex
approved these changes
Mar 16, 2026
There was a problem hiding this comment.
♻️ Duplicate comments (1)
app/evaluations/[id]/page.tsx (1)
48-60:⚠️ Potential issue | 🟡 MinorHandle carriage returns in CSV escaping.
escapeCSVValueonly replaces\n(LF), leaving\r(CR) intact. This can cause issues with Windows-style line endings (\r\n) or standalone carriage returns, potentially breaking CSV structure in some parsers.Proposed fix
const escapeCSVValue = (value: string): string => { - return value.replace(/"/g, '""').replace(/\n/g, ' '); + return value.replace(/"/g, '""').replace(/[\r\n]+/g, ' '); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/evaluations/`[id]/page.tsx around lines 48 - 60, escapeCSVValue currently only replaces LF (\n) and misses CR (\r), so update escapeCSVValue to normalize all line endings by replacing CRLF, CR, or LF with a single space (e.g. use value.replace(/\r\n|\r|\n/g, ' ')). Keep sanitizeCSVCell behavior the same (it should call the updated escapeCSVValue and then apply the preventFormulaInjection check on the returned sanitized string before wrapping it in quotes), referencing the escapeCSVValue and sanitizeCSVCell functions.
🧹 Nitpick comments (1)
app/evaluations/[id]/page.tsx (1)
251-256: Consider escaping score values for robustness.Score values are pushed without CSV escaping. While numeric values and typical categorical strings (e.g., "PASS", "FAIL") are safe, custom categorical values containing commas or quotes would break CSV structure.
Proposed fix
// Scores with comments scoreNames.forEach(name => { const score = group.scores[i]?.find(s => s.name === name); - row.push(score ? String(score.value) : ''); + row.push(score ? sanitizeCSVCell(String(score.value)) : ''); row.push(score?.comment ? sanitizeCSVCell(score.comment, true) : ''); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/evaluations/`[id]/page.tsx around lines 251 - 256, The CSV export currently pushes raw score values into row (inside the scoreNames.forEach loop), which can break CSV when values contain commas/quotes; update the loop that iterates scoreNames (and accesses group.scores[i]?.find) to run each score.value through the existing sanitizeCSVCell utility (same way comments are sanitized) before pushing, e.g. replace the raw String(score.value) push with a sanitized version via sanitizeCSVCell(score.value, true) while keeping the conditional empty-string behavior when score is missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/evaluations/`[id]/page.tsx:
- Around line 48-60: escapeCSVValue currently only replaces LF (\n) and misses
CR (\r), so update escapeCSVValue to normalize all line endings by replacing
CRLF, CR, or LF with a single space (e.g. use value.replace(/\r\n|\r|\n/g, '
')). Keep sanitizeCSVCell behavior the same (it should call the updated
escapeCSVValue and then apply the preventFormulaInjection check on the returned
sanitized string before wrapping it in quotes), referencing the escapeCSVValue
and sanitizeCSVCell functions.
---
Nitpick comments:
In `@app/evaluations/`[id]/page.tsx:
- Around line 251-256: The CSV export currently pushes raw score values into row
(inside the scoreNames.forEach loop), which can break CSV when values contain
commas/quotes; update the loop that iterates scoreNames (and accesses
group.scores[i]?.find) to run each score.value through the existing
sanitizeCSVCell utility (same way comments are sanitized) before pushing, e.g.
replace the raw String(score.value) push with a sanitized version via
sanitizeCSVCell(score.value, true) while keeping the conditional empty-string
behavior when score is missing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c5dbbd18-9a4b-453e-a3b9-194624847e07
📒 Files selected for processing (1)
app/evaluations/[id]/page.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Target issue is #67
Summary by CodeRabbit