CSV export #2797
CSV export #2797
Conversation
Console (appwrite/console)Project ID: Sites (1)
Tip Storage files get ClamAV malware scanning and encryption by default |
WalkthroughUpdates introduce CSV export functionality and related UI/analytics: a new CSV export wizard page for tables; a runtime CSV export progress component (csvExportBox.svelte) with real-time migration updates, download links, and error modal; added CSV export analytics enum entries; Export/Import CSV buttons and an expand/collapse control in the table header; CsvExportBox exported from the components barrel; a new Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 4
🤖 Fix all issues with AI agents
In `@src/lib/components/csvExportBox.svelte`:
- Around line 26-32: The automatic call to window.open in downloadExportedFile
can be blocked by popup blockers; change the behavior to not auto-open but
instead expose the download URL to the component state and render a visible
fallback (e.g., a "Download CSV" anchor/button) when the URL becomes available,
and only call window.open() (or programmatic click) in response to a user
gesture via that button; update any code paths that invoke downloadExportedFile
(status 'completed' handling) to set the URL state instead of directly opening
it and ensure the UI shows the fallback link/button bound to
downloadExportedFile.
- Around line 121-133: The text() function is injecting unescaped tableName into
an HTML string used with {`@html`}, creating an XSS risk; fix by either
HTML-escaping tableName inside text() (replace &, <, >, ", ' with entities
before wrapping in <b>) or, better, stop returning HTML from text() and instead
return plain strings and render the bold label in the Svelte template (e.g. use
<b>{tableName}</b> in the markup and remove {`@html` text(...)}), and update all
call sites that use {`@html` text(...)} to the new safe rendering approach; the
function name to change is text() and the template usage to update is where
{`@html`} renders its result.
- Around line 135-145: The initial fetch in onMount calling
sdk.forProject(...).migrations.list(...) lacks error handling; update the
onMount logic (the sdk.forProject(...).migrations.list call) to handle failures
by adding a .catch(...) to the promise chain (or convert to async/await with
try/catch) and call a handler such as processLogger.error or set a component
error state; ensure the catch logs the error and does not leave the UI in a
silent failed state while still calling updateOrAddItem in the successful .then
branch.
In
`@src/routes/`(console)/project-[region]-[project]/databases/database-[database]/table-[table]/export/+page.svelte:
- Around line 56-68: The constructed tableUrl currently appends raw query text
from page.url.searchParams.get('query') which can break URLs; update the
tableUrl derivation (the const tableUrl = $derived.by(...) block) to URL-encode
the query using encodeURIComponent (same approach as getTableExportUrl) when
building the returned string so the query param is safe (i.e., call
encodeURIComponent on the queryParam before interpolating into
`${url}?query=...`).
🧹 Nitpick comments (2)
src/routes/(console)/project-[region]-[project]/+layout.svelte (1)
2-2: Inconsistent import style for CSV components.
CsvExportBoxis imported from the barrel ($lib/components), butCsvImportBoxis imported directly from its file path. For consistency and per coding guidelines, consider importingCsvImportBoxfrom the barrel as well.Suggested fix
- import { BackupRestoreBox, MigrationBox, UploadBox, CsvExportBox } from '$lib/components'; + import { BackupRestoreBox, MigrationBox, UploadBox, CsvExportBox, CsvImportBox } from '$lib/components'; ... -import CsvImportBox from '$lib/components/csvImportBox.svelte';Based on coding guidelines: use
$libalias for module imports.Also applies to: 27-27
src/lib/components/csvExportBox.svelte (1)
51-77: Redundant variable and expression.
existing(line 65) duplicatescurrent(line 55). Also,tableNameis assigned fromcurrent?.tableon line 56, makingtableName ?? current?.tableon line 81 always evaluate tocurrent?.table.Suggested cleanup
async function updateOrAddItem(exportData: Payload | Models.Migration) { if (exportData.destination?.toLowerCase() !== 'csv') return; const status = exportData.status; const current = exportItems.get(exportData.$id); - let tableName = current?.table; // Get bucket, filename, and download URL from migration options const options = ('options' in exportData ? exportData.options : {}) || {}; const bucketId = options.bucketId || ''; const fileName = options.filename || ''; const downloadUrl = options.downloadUrl || ''; let bucketName = current?.bucketName; - const existing = exportItems.get(exportData.$id); - const isDone = (s: string) => ['completed', 'failed'].includes(s); const isInProgress = (s: string) => ['pending', 'processing'].includes(s); // Skip if we're trying to set an in-progress status on a completed migration - const shouldSkip = existing && isDone(existing.status) && isInProgress(status); + const shouldSkip = current && isDone(current.status) && isInProgress(status); const hasNewData = - downloadUrl && (!existing?.downloadUrl || existing.downloadUrl !== downloadUrl); - const shouldSkipDuplicate = existing?.status === status && !hasNewData; + downloadUrl && (!current?.downloadUrl || current.downloadUrl !== downloadUrl); + const shouldSkipDuplicate = current?.status === status && !hasNewData; if (shouldSkip || shouldSkipDuplicate) return; exportItems.set(exportData.$id, { status, - table: tableName ?? current?.table, + table: current?.table,
...)/project-[region]-[project]/databases/database-[database]/table-[table]/export/+page.svelte
Show resolved
Hide resolved
…base-[database]/table-[table]/export/+page.svelte Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@src/routes/`(console)/project-[region]-[project]/databases/database-[database]/table-[table]/export/+page.svelte:
- Around line 121-125: The catch block that calls addNotification({ type:
'error', message: error.message }) may assume the caught value is an Error;
update the handler used in the try/catch around the export logic so it safely
extracts a message (e.g., if (error instanceof Error) use error.message else use
String(error) or a default like 'Unknown error') before passing to
addNotification, ensuring the message field is always a string; locate the catch
and addNotification call in the export flow (the catch block shown) to implement
this safe fallback.
- Around line 70-73: In removeLocalFilter, after deleting the tag from
localQueries and before/after recreating the Map, detect if localQueries is
empty and if so set exportWithFilters = false so the checkbox is automatically
unchecked when the last filter is removed; update the function that references
localQueries (removeLocalFilter) to clear exportWithFilters when
localQueries.size === 0 to ensure the UI disables and unchecks the
export-with-filters checkbox.
...)/project-[region]-[project]/databases/database-[database]/table-[table]/export/+page.svelte
Show resolved
Hide resolved
...)/project-[region]-[project]/databases/database-[database]/table-[table]/export/+page.svelte
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/lib/components/csvExportBox.svelte`:
- Around line 34-41: The showErrorNotification function currently indexes
payload.errors[0] without confirming payload.errors exists which can throw;
update showErrorNotification to defensively check that payload.errors is an
array and has at least one element before attempting to parse or read
payload.errors[0] (e.g., use Array.isArray(payload.errors) &&
payload.errors.length > 0), and fall back to the default errorMessage when that
check fails; ensure both the try and catch branches reference the guarded value
so neither JSON.parse nor the fallback reads an undefined index.
♻️ Duplicate comments (3)
src/lib/components/csvExportBox.svelte (3)
91-105: Auto-open download still risks popup blockers; prefer user-gesture only.
132-144: Avoid {@html} with unescapedtableName(XSS risk).
146-156: Handle migrations.list errors to avoid silent failures.
🧹 Nitpick comments (2)
src/lib/components/csvExportBox.svelte (2)
1-10: Align component filename/location with PascalCase + feature folder.Current file path
src/lib/components/csvExportBox.svelteis camelCase and not in a feature subdirectory. Please rename to PascalCase and place it under a feature folder (e.g.,src/lib/components/csv/CsvExportBox.svelte) and update imports/exports accordingly. As per coding guidelines and learnings, ...
181-189: Consider addingtype="button"to header controls.Avoid unintended form submissions if this component is ever rendered inside a
<form>.♻️ Suggested tweak
- <button + <button + type="button" class="upload-box-button" class:is-open={isOpen} aria-label="toggle upload box" onclick={() => (isOpen = !isOpen)}> @@ - <button class="upload-box-button" aria-label="close export box" onclick={clear}> + <button + type="button" + class="upload-box-button" + aria-label="close export box" + onclick={clear}>
| async function showErrorNotification(payload: Payload) { | ||
| let errorMessage = 'Export failed. Please try again.'; | ||
| try { | ||
| const parsed = JSON.parse(payload.errors[0]); | ||
| errorMessage = parsed?.message || errorMessage; | ||
| } catch { | ||
| errorMessage = payload.errors[0] || errorMessage; | ||
| } |
There was a problem hiding this comment.
Guard against missing payload.errors before indexing.
If payload.errors is undefined, the catch block still accesses payload.errors[0] and can throw. Add a defensive guard.
💡 Suggested fix
async function showErrorNotification(payload: Payload) {
let errorMessage = 'Export failed. Please try again.';
+ const rawError = payload.errors?.[0];
try {
- const parsed = JSON.parse(payload.errors[0]);
- errorMessage = parsed?.message || errorMessage;
+ if (rawError) {
+ const parsed = JSON.parse(rawError);
+ errorMessage = parsed?.message || rawError || errorMessage;
+ }
} catch {
- errorMessage = payload.errors[0] || errorMessage;
+ errorMessage = rawError || errorMessage;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function showErrorNotification(payload: Payload) { | |
| let errorMessage = 'Export failed. Please try again.'; | |
| try { | |
| const parsed = JSON.parse(payload.errors[0]); | |
| errorMessage = parsed?.message || errorMessage; | |
| } catch { | |
| errorMessage = payload.errors[0] || errorMessage; | |
| } | |
| async function showErrorNotification(payload: Payload) { | |
| let errorMessage = 'Export failed. Please try again.'; | |
| const rawError = payload.errors?.[0]; | |
| try { | |
| if (rawError) { | |
| const parsed = JSON.parse(rawError); | |
| errorMessage = parsed?.message || rawError || errorMessage; | |
| } | |
| } catch { | |
| errorMessage = rawError || errorMessage; | |
| } |
🤖 Prompt for AI Agents
In `@src/lib/components/csvExportBox.svelte` around lines 34 - 41, The
showErrorNotification function currently indexes payload.errors[0] without
confirming payload.errors exists which can throw; update showErrorNotification
to defensively check that payload.errors is an array and has at least one
element before attempting to parse or read payload.errors[0] (e.g., use
Array.isArray(payload.errors) && payload.errors.length > 0), and fall back to
the default errorMessage when that check fails; ensure both the try and catch
branches reference the guarded value so neither JSON.parse nor the fallback
reads an undefined index.
What does this PR do?
(Provide a description of what this PR does.)
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work.)
Related PRs and Issues
(If this PR is related to any other PR or resolves any issue or related to any issue link all related PR and issues here.)
Have you read the Contributing Guidelines on issues?
(Write your answer here.)
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.