Add filtered CSV exports for Jobs and Documents - #684
Conversation
📝 WalkthroughWalkthroughAdds a reusable CSV utility with formula-value sanitization and browser downloads. The documents admin list gains text/date filtering and filtered CSV export, while the jobs admin list gains partition filtering and filtered CSV export with accompanying tests. ChangesAdmin CSV exports
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant AdminList
participant downloadCsv
participant toExcelCsv
participant Browser
AdminList->>downloadCsv: pass filtered rows and export filename
downloadCsv->>toExcelCsv: serialize columns and rows
toExcelCsv-->>downloadCsv: return sanitized CSV text
downloadCsv->>Browser: create Blob and trigger file download
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: da366a65b5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac691dbc4b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![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
🧹 Nitpick comments (1)
ui/src/lib/csv.ts (1)
21-29: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd a Byte Order Mark (BOM) to ensure correct UTF-8 decoding in Excel.
When downloading CSVs, Microsoft Excel typically ignores the
charset=utf-8MIME parameter and defaults to the system's local codepage, which can mangle non-ASCII characters (e.g., accents, emojis, and non-Latin text). Prepending the UTF-8 BOM (\uFEFF) to the Blob content explicitly instructs Excel to parse the file as UTF-8.♻️ Proposed fix
export function downloadCsv<T>(filename: string, columns: CsvColumn<T>[], rows: T[]) { - const blob = new Blob([toCsv(columns, rows)], { type: "text/csv;charset=utf-8" }); + const blob = new Blob(["\uFEFF", toCsv(columns, rows)], { type: "text/csv;charset=utf-8" }); const url = URL.createObjectURL(blob);🤖 Prompt for 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. In `@ui/src/lib/csv.ts` around lines 21 - 29, Update downloadCsv to prepend the UTF-8 BOM character to the CSV content before constructing the Blob, while preserving the existing text/csv;charset=utf-8 MIME type and download flow.
🤖 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 `@ui/src/pages/admin/documents/list.tsx`:
- Around line 381-389: Add an explicit accessible name to the search Input in
the file-search control by setting an aria-label that identifies its purpose,
while preserving the existing placeholder, value, and onChange behavior.
In `@ui/src/pages/admin/jobs/list.tsx`:
- Line 63: Update handleStatusTabChange to reset partitionFilter to "__all__"
whenever the status tab changes, matching the existing search-input reset
behavior and ensuring the selected partition remains valid for the new tab.
---
Nitpick comments:
In `@ui/src/lib/csv.ts`:
- Around line 21-29: Update downloadCsv to prepend the UTF-8 BOM character to
the CSV content before constructing the Blob, while preserving the existing
text/csv;charset=utf-8 MIME type and download flow.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 852d69b0-9522-4ce0-ae2a-37176fc71d3f
📒 Files selected for processing (6)
ui/src/lib/csv.test.tsui/src/lib/csv.tsui/src/pages/admin/documents/list.test.tsxui/src/pages/admin/documents/list.tsxui/src/pages/admin/jobs/list.test.tsxui/src/pages/admin/jobs/list.tsx
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 877bd2a2cb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
e2322c0 to
f010758
Compare
f010758 to
8cbd32a
Compare
andyne13
left a comment
andyne13
left a comment
There was a problem hiding this comment.
Reviewed end-to-end (csv util + both list pages + tests), and traced the data paths through the backend. CI is green and the tests are well-targeted. Nice, focused change — the CSV formula-injection handling in particular is solid: csvCell prefixes ' for = + - @ \t \r \n leading cells and RFC-4180-quotes anything containing " , \n \r, and csv.test.ts covers the injection vectors. The __openrag/all_partitions__ sentinel is a good fix for the earlier __all__ collision concern, and there's even a test with a real partition literally named __all__.
A few things worth addressing before merge, plus some nits. I verified each against the code.
Should-fix (all hit the French/Excel case)
-
CSV has no UTF-8 BOM → accented filenames garble in Excel.
downloadCsv(ui/src/lib/csv.ts) buildsnew Blob([...], { type: "text/csv;charset=utf-8" }), but Excel ignores the blob's charset and decodes by system locale, so a file likeRapport_privé.pdfbecomes mojibake. One-char fix: prependto the content. -
Hardcoded
,delimiter → single-column open in;-locale Excel.toCsvjoins cells with,. In a locale whose list separator is;(French is the common one), Excel drops the whole row into a single column. Worth a configurable delimiter or a leadingsep=,hint line. -
Jobs CSV omits
created_at/duration_ms, which are actually exposed. The PR's "Limitation" note says the queue task API doesn't expose created/updated timestamps, butTaskListItemdeclarescreated_at?: string | nullandduration_ms?: number | null(ui/src/lib/api/jobs.ts:44-45) and the route returns both (openrag/api/routers/admin/jobs.py:110-111). So a job timestamp column looks feasible in the export, and date-filtering for jobs may be more attainable than the note implies — worth a second look.
Nits / follow-ups
-
Date filter is timezone-fragile.
indexed_atis storedDateTime(timezone=True)and serialized as a UTC ISO string;documents/list.tsxcomparesstr(file.indexed_at ?? file.created_at).slice(0,10)(the UTC calendar date) against<input type="date">(a local date). Near midnight in UTC+1/+2 this can be off by a day (e.g. a file at…T23:30:00+00:00is "yesterday" in UTC but "today" locally). Minor, but user-visible for a French deployment. -
Unbounded fetch + in-memory CSV.
list_partition_filesrunsSELECT * ... WHERE partition_name = $1with noLIMITwhen the caller passes none (the page does), andlistTasksisn't paginated either — so the client fetches every row, filters in memory, andtoCsvbuilds one string. Fine at hundreds; a memory/UI ceiling worth noting for large partitions. Pre-existing; the export just makes it more consequential. -
downloadCsvbrowser robustness. It revokes the object URL synchronously right afterlink.click()— some browsers cancel the download if the URL is revoked too early (safer insidesetTimeout(() => URL.revokeObjectURL(url), 0)), the<a>is never appended to the DOM (historically Firefox required it for a programmatic click), and there's no error handling/toast if it throws — the rest of these pages surface failures viatoast. -
mimetypecolumn/search is sparse (not wrong, just FYI).mimetypelands infile_metadataonly for MCP-ingested files (mcp_servicesetsmetadata["mimetype"] = guessed_mime) or uploads that pass it explicitly; the regular upload path (_build_metadata) doesn't set it. So themimetypeCSV column and the "search by mimetype" branch will be blank for files uploaded through the admin UI. Expected, but worth knowing the column won't populate for the common upload flow. -
Trivial: rows are joined with
\n(RFC 4180 specifies CRLF; Excel tolerates LF), and thestr()helper is duplicated across both list files.
Everything above is verified against the code. Overall this is good work — my main asks are #1–#3 (all Excel/French-locale), the rest are optional.
|
Thanks for the careful review. I addressed the three main asks: exported CSVs now open correctly in Excel across UTF-8 and locale-specific separators, and Jobs exports include creation time and duration. I also fixed local-day date filtering and made browser downloads clean up safely while reporting failures. I left pagination and server-side export for a follow-up because they change the API and loading model beyond this PR. The MIME metadata note is already covered by the current |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@ui/src/lib/csv.test.ts`:
- Around line 40-44: Remove the duplicated column-definition argument in the
toExcelCsv test and the duplicated function invocation in the downloadCsv test.
Keep one valid call for each test while preserving the existing assertions and
test data.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 823d189e-2775-4088-9025-b9f9e61ba45b
📒 Files selected for processing (6)
ui/src/lib/csv.test.tsui/src/lib/csv.tsui/src/pages/admin/documents/list.test.tsxui/src/pages/admin/documents/list.tsxui/src/pages/admin/jobs/list.test.tsxui/src/pages/admin/jobs/list.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- ui/src/pages/admin/jobs/list.test.tsx
- ui/src/pages/admin/documents/list.test.tsx
- ui/src/pages/admin/documents/list.tsx
- ui/src/pages/admin/jobs/list.tsx
andyne13
left a comment
andyne13
left a comment
There was a problem hiding this comment.
Thanks for the thorough follow-up — verified 61b236ea against each review point and they're all genuinely addressed in code, with tests:
- UTF-8 BOM +
sep=,line (toExcelCsv) — accented filenames and;-locale Excel both open correctly; covered by thetoExcelCsvtest. - Jobs export now includes
created_at/duration_ms. - Date filter — local-midnight epoch comparison fixes the timezone off-by-one.
downloadCsv— anchor appended/removed, deferredrevokeObjectURL,try/finallycleanup, and both callers report failures via toast; the download test locks the DOM-during-click / no-sync-revoke / post-timer cleanup behaviour.
Pagination/server-side export as a follow-up is reasonable, and the mimetype column is fine (populated for MCP-ingested files). CI is green. LGTM 👍
2 participants
Context
Investigating benchmark runs or failed files currently requires paging through the Admin UI or falling back to scripts.
Change
Extend the existing Jobs and Documents controls without changing the table layout:
Partially addresses #546.
Limitation
Job date filtering remains outside this PR. Job creation time and duration are available and included in the CSV export, but adding another UI filter should be scoped separately.
Validation
Summary by CodeRabbit