feat: add client-side CSV download option to SQL playground results#562
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds client-side CSV download for SQL query results (escaping, Blob creation, and browser download) and imports the ChangesSQL Result CSV Export and CORS import
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@client/src/module/student/sql/components/SqlResultTable.tsx`:
- Around line 17-27: The CSV builder in SqlResultTable.tsx currently builds
headers without escaping embedded quotes and joins lines with '\n'; update the
headers construction (symbol: headers) to quote and double any embedded quotes
the same way csvRows does (i.e., replace " with "" inside each column name) and
switch the final join for the CSV (symbol: csv) to use CRLF line endings '\r\n'
instead of '\n' so output conforms to RFC4180; keep the existing quoting logic
used in csvRows for consistency.
- Around line 86-103: Replace the native <button> in SqlResultTable with the
shared Button component from client/src/components/ui/button.tsx: import Button
into SqlResultTable.tsx, swap the native element used around the CSV icon and
"CSV" label with <Button onClick={handleDownloadCSV} title="Download results as
CSV" ...> preserving the existing className, children (the SVG icon and "CSV"
text) and any accessibility attributes, and remove the old native button; ensure
the Button receives the same click handler and visual/styling props (or map to
Button's variant/size props if required) so behavior and appearance remain
unchanged.
In `@server/src/index.ts`:
- Around line 98-101: There are two conflicting CORS layers: the top-level
app.use(cors({...})) and the manual CORS middleware plus the ingest route that
sets Access-Control-Allow-Origin: *; remove or disable the early app.use(cors({
origin: 'http://localhost:5173', credentials: true })) and instead apply cors()
only where needed (either configure cors() per-route or keep the manual
allowlist middleware and the ingest route's wildcard behavior), ensuring the
ingest route does not get Access-Control-Allow-Credentials: true while using
Access-Control-Allow-Origin: *; update code references: remove or change the
top-level app.use(cors(...)) and adjust the manual CORS middleware and the
ingest route handler to use a single consistent cors strategy.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9a5c817a-5994-4997-808e-c0d7d686174f
📒 Files selected for processing (2)
client/src/module/student/sql/components/SqlResultTable.tsxserver/src/index.ts
Sachinchaurasiya360
left a comment
Sachinchaurasiya360
left a comment
There was a problem hiding this comment.
The CSV download feature itself is well-scoped and the RFC 4180 escaping logic is a good approach. However, there are blocking issues before this can merge:
1. CORS middleware addition must be removed (blocker)
The change to server/src/index.ts adds a bare app.use(cors()) call, creating a duplicate CORS layer that conflicts with the existing CORS configuration already in place. The original issue (#469) explicitly stated "no server change needed" for this feature. Please revert the server/src/index.ts change entirely — it is out of scope and breaks the existing CORS setup.
2. CSV header row escaping missing
The RFC 4180 escaping is applied to data rows but the header row (column names) bypasses the escaper. Column names containing commas or quotes would produce malformed CSVs. Apply the same escapeCSV function to header values.
3. Use the project's Button component
The download trigger button should use Button from client/src/components/ui/button.tsx (variant: secondary or ghost) instead of a raw <button> with inline Tailwind classes. This is a project-wide convention.
4. Line endings
CSV RFC 4180 requires CRLF (\r\n) line endings, not LF. Update the row separator.
Please fix items 1-4 and the PR will be approved.
|
Hi! Thanks for the guidance. I have updated the PR to meet the repository specifications. |
Sachinchaurasiya360
left a comment
Sachinchaurasiya360
left a comment
There was a problem hiding this comment.
All four blockers addressed: CORS middleware removed, header row escaping added, project Button component used, CRLF line endings per RFC 4180. Clean implementation. Approved.
Sachinchaurasiya360
added
gssoc:approved
2 participants
Pull Request
Description
Implements a client-side CSV extraction and download utility within the SQL playground results component (SqlResultTable.tsx).
Key implementations:
Related Issue
Fixes #469
Type of Change
Testing
Screenshots
Checklist
Summary by CodeRabbit