feat: show CSV import error details with inline indicator and modal #2401
Conversation
ConsoleProject ID: Sites (2)
Note You can use Avatars API to generate QR code for any text or URLs. |
WalkthroughAdds an optional errors?: string[] to ImportItem and surfaces import errors in the UI. Failed items render an inline error row with an exclamation icon and a "View details" link that opens an "Import error" modal showing parsed/pretty-printed JSON (falls back to raw text), with copy and scroll support. Adjusts graph height for completed/failed statuses (100 → 60), updates some status text, and widens upload-box-content. Also updates link component to forward $$restProps to its rendered anchor or button. Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (10)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib/components/csvImportBox.svelte (2)
108-123: Do not skip updates when only errors change (failed state).If a second payload arrives with the same status but with error details, the current skip logic prevents updating the map, so the "View details" modal may never show content.
Apply this diff to consider error changes when deciding to skip:
- const isDone = (s: string) => s === 'completed' || s === 'failed'; - const isInProgress = (s: string) => ['pending', 'processing', 'uploading'].includes(s); - - const shouldSkip = - (existing && isDone(existing.status) && isInProgress(status)) || - existing?.status === status; + const isDone = (s: string) => s === 'completed' || s === 'failed'; + const isInProgress = (s: string) => ['pending', 'processing', 'uploading'].includes(s); + const incomingErrors = Array.isArray((importData as Payload).errors) + ? ((importData as Payload).errors as string[]) + : undefined; + const errorsEqual = + JSON.stringify(existing?.errors ?? []) === JSON.stringify(incomingErrors ?? []); + const shouldSkip = + (existing && isDone(existing.status) && isInProgress(status)) || + (existing?.status === status && errorsEqual); if (shouldSkip) return items; const next = new Map(items); - const errors = Array.isArray((importData as Payload).errors) - ? ((importData as Payload).errors as string[]) - : undefined; - next.set(importData.$id, { status, table: tableName ?? undefined, errors }); + next.set(importData.$id, { + status, + table: tableName ?? undefined, + errors: incomingErrors + }); return next;
153-164: Sanitize table name to prevent XSS via {@html}.Table names are user-controlled. Rendering with {@html} and embedding the name directly is an XSS risk.
Apply this diff to sanitize the name inside the text builder:
function text(status: string, collectionName = '') { - const name = collectionName ? `<b>${collectionName}</b>` : ''; + const name = collectionName ? `<b>${escapeHtml(collectionName)}</b>` : ''; switch (status) { case 'completed': - case 'failed': - return `Importing CSV file${name ? ` to ${name}` : ''}`; + return `CSV import finished${name ? ` for ${name}` : ''}.`; + case 'failed': + return `CSV import failed${name ? ` for ${name}` : ''}.`; case 'processing': return `Importing CSV file${name ? ` to ${name}` : ''}`; default: return 'Preparing CSV for import...'; } }Add this helper (outside this range, e.g., near other helpers):
function escapeHtml(value: string): string { const map: Record<string, string> = { '&': '&', '<': '<', '>': '>', '"': '"', "'": ''', '/': '/', '`': '`', '=': '=' }; return value.replace(/[&<>"'`=\/]/g, (ch) => map[ch] ?? ch); }
🧹 Nitpick comments (4)
src/lib/components/csvImportBox.svelte (4)
192-195: Avoid opening an empty modal when no errors are available.Guard against undefined/empty arrays.
Apply this diff:
- function openDetails(errors: string[] | undefined) { - selectedErrors = errors ?? []; - showDetails = true; - } + function openDetails(errors: string[] | undefined) { + if (!errors?.length) return; + selectedErrors = errors; + showDetails = true; + }
257-263: Render “View details” only when errors exist.Prevents a no-op click and a blank modal.
Apply this diff:
- There was an import issue. - <button - class="link-button" - on:click={() => openDetails(value.errors)} - >View details</button> + There was an import issue. + {#if value.errors && value.errors.length} + <button + class="link-button" + on:click={() => openDetails(value.errors)} + >View details</button> + {/if}
276-282: Pluralize modal title based on error count.Minor UX polish.
Apply this diff:
-<Modal title="Import error" bind:show={showDetails} hideFooter> +<Modal + title={selectedErrors.length > 1 ? 'Import errors' : 'Import error'} + bind:show={showDetails} + hideFooter>
327-334: Add focus-visible style for keyboard accessibility.Custom link-styled button currently lacks a visible focus indicator.
Apply this diff:
.link-button { background: none; border: none; padding: 0; color: var(--fgcolor-error); text-decoration: underline; cursor: pointer; } + .link-button:focus-visible { + outline: 2px solid var(--outline-focus, #5b9dd9); + outline-offset: 2px; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lib/components/csvImportBox.svelte(10 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: e2e
- GitHub Check: build
🔇 Additional comments (4)
src/lib/components/csvImportBox.svelte (4)
11-13: LGTM: Correct imports for UI components.Imports align with usage below.
23-24: LGTM: ImportItem.errors added and optionally propagated.Type change is consistent with usage in the store updates and UI.
147-147: Verify: Completed/failed progress at 60% looks incomplete.UX-wise, completed is usually 100%. Please confirm this change with design; otherwise revert to 100%.
Suggested change:
- return 60; + return 100;
304-305: LGTM: Minor width tweak.No concerns.
| case 'completed': | ||
| case 'failed': | ||
| return `Import to ${name} ${status}`; | ||
| return `Importing CSV file${name ? ` to ${name}` : ''}`; | ||
| case 'processing': | ||
| return `Importing CSV file${name ? ` to ${name}` : ''}`; | ||
| default: |
There was a problem hiding this comment.
Copy fix: Completed/failed messages shouldn't say "Importing".
Use explicit completed/failed text for clarity.
Apply this diff:
- case 'completed':
- case 'failed':
- return `Importing CSV file${name ? ` to ${name}` : ''}`;
+ case 'completed':
+ return `CSV import finished${name ? ` for ${name}` : ''}.`;
+ case 'failed':
+ return `CSV import failed${name ? ` for ${name}` : ''}.`;📝 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.
| case 'completed': | |
| case 'failed': | |
| return `Import to ${name} ${status}`; | |
| return `Importing CSV file${name ? ` to ${name}` : ''}`; | |
| case 'processing': | |
| return `Importing CSV file${name ? ` to ${name}` : ''}`; | |
| default: | |
| case 'completed': | |
| return `CSV import finished${name ? ` for ${name}` : ''}.`; | |
| case 'failed': | |
| return `CSV import failed${name ? ` for ${name}` : ''}.`; | |
| case 'processing': | |
| return `Importing CSV file${name ? ` to ${name}` : ''}`; | |
| default: |
🤖 Prompt for AI Agents
In src/lib/components/csvImportBox.svelte around lines 156 to 161, the
'completed' and 'failed' cases currently return "Importing CSV file...", which
is incorrect; update those two cases to return explicit end-state messages
(e.g., "CSV import completed" when 'completed' and "CSV import failed" when
'failed'), preserving the existing optional "to {name}" interpolation when name
is present; leave the 'processing' case as-is returning the ongoing "Importing
CSV file" message.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| case 'completed': | ||
| case 'failed': | ||
| return 100; | ||
| return 60; |
There was a problem hiding this comment.
should be 100% because the process is completed.
There was a problem hiding this comment.
why was this marked resolved?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/lib/elements/link.svelte (1)
30-44: Avoid empty target/rel attributes when not external.Prefer omitting attributes over empty strings to keep DOM clean and avoid oddities with target="".
- target={external ? '_blank' : ''} - rel={external ? 'noopener noreferrer' : ''}> + target={external ? '_blank' : undefined} + rel={external ? 'noopener noreferrer' : undefined}>src/lib/components/csvImportBox.svelte (1)
119-123: Details UX: guard empty errors, cap large payloads, and add modal fallback.
- Hide/disable the link when no errors exist, and show a count to set expectations.
- Cap the number of errors rendered to avoid massive strings freezing the UI.
- Provide a friendly fallback in the modal when there’s nothing to show.
@@ - let showDetails = false; - let selectedErrors: string[] = []; + let showDetails = false; + let selectedErrors: string[] = []; + const MAX_ERRORS = 100; // avoid rendering extremely large payloads @@ - function openDetails(errors: string[] | undefined) { - selectedErrors = errors ?? []; + function openDetails(errors: string[] | undefined) { + selectedErrors = (errors ?? []).slice(0, MAX_ERRORS); showDetails = true; } @@ - {#if value.status === 'failed'} + {#if value.status === 'failed'} <Layout.Stack direction="row" gap="xs" alignItems="center" inline> @@ - <Typography.Text color="--fgcolor-error"> + <Typography.Text color="--fgcolor-error"> There was an import issue. - <Link - style="color: inherit" - on:click={() => openDetails(value.errors)} - >View details</Link> + {#if value.errors?.length} + <Link + style="color: inherit" + on:click={() => openDetails(value.errors)}> + View details ({value.errors.length}) + </Link> + {:else} + <span>No details available</span> + {/if} </Typography.Text> </Layout.Stack> {/if} @@ -<Modal title="Import error" bind:show={showDetails} hideFooter> - <Layout.Stack gap="m"> - <Layout.Stack> - <Code language="json" code={parsedErrors.join('\n\n')} withCopy allowScroll /> - </Layout.Stack> - </Layout.Stack> -</Modal> +<Modal title="Import error" bind:show={showDetails} hideFooter> + <Layout.Stack gap="m"> + <Layout.Stack> + {#if parsedErrors.length} + <Code language="json" code={parsedErrors.join('\n\n')} withCopy allowScroll /> + {:else} + <Typography.Text>No error details available.</Typography.Text> + {/if} + </Layout.Stack> + </Layout.Stack> +</Modal>Also applies to: 190-205, 248-266, 277-283
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/lib/components/csvImportBox.svelte(8 hunks)src/lib/elements/link.svelte(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: e2e
- GitHub Check: build
🔇 Additional comments (4)
src/lib/elements/link.svelte (1)
32-32: Nice: passthrough props/events now supported on both branches.Spreading $$restProps on Anchor/Button enables aria-/data-/class and user handlers while keeping your own tracking intact. Good ordering (explicit props after spread) prevents accidental overrides.
Also applies to: 46-55
src/lib/components/csvImportBox.svelte (3)
154-162: Copy fix: completed/failed shouldn’t say “Importing”.Use explicit end-state messages.
function text(status: string, collectionName = '') { const name = collectionName ? `<b>${collectionName}</b>` : ''; switch (status) { - case 'completed': - case 'failed': - return `Importing CSV file${name ? ` to ${name}` : ''}`; + case 'completed': + return `CSV import finished${name ? ` for ${name}` : ''}.`; + case 'failed': + return `CSV import failed${name ? ` for ${name}` : ''}.`; case 'processing': return `Importing CSV file${name ? ` to ${name}` : ''}`; default: return 'Preparing CSV for import...'; } }
148-148: Graph height tweak LGTM.Reducing completed/failed bar height to 60% aligns with the new inline error row and saves vertical space.
305-305: Width increase: verify layout on narrow containers.324px may overflow in tighter sidebars. Confirm it doesn’t cause horizontal scroll at common breakpoints or with long table names.
Consider making width responsive (e.g., max-width: 100%;) if you spot overflow in QA.
| // re-render the key for sheet UI. | ||
| import { hash } from '$lib/helpers/string'; | ||
| import { spreadsheetRenderKey } from '$routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/store'; | ||
| import Link from '$lib/elements/link.svelte'; |
There was a problem hiding this comment.
use import { Link } from '$lib/elements';
| case 'completed': | ||
| case 'failed': | ||
| return `Import to ${name} ${status}`; | ||
| return `Importing CSV file${name ? ` to ${name}` : ''}`; | ||
| case 'processing': | ||
| return `Importing CSV file${name ? ` to ${name}` : ''}`; | ||
| default: |
| const next = new Map(items); | ||
| next.set(importData.$id, { status, table: tableName ?? undefined }); | ||
| const errors = Array.isArray((importData as Payload).errors) |
There was a problem hiding this comment.
don't think we need to cast here. we have a similar check above for showCompletionNotification, we can make a helper function to return the error instead of hard type casts and duplicating the logic.
| case 'completed': | ||
| case 'failed': | ||
| return 100; | ||
| return 60; |
There was a problem hiding this comment.
why was this marked resolved?
| parsedErrors = selectedErrors.map((err) => { | ||
| try { | ||
| return JSON.stringify(JSON.parse(err), null, 2); | ||
| } catch { | ||
| return err; | ||
| } | ||
| }); |
There was a problem hiding this comment.
as mentioned above, we should extract this logic to a function since its 2/3 used now.
| <Modal title="Import error" bind:show={showDetails} hideFooter> | ||
| <Layout.Stack gap="m"> | ||
| <Layout.Stack> | ||
| <Code language="json" code={parsedErrors.join('\n\n')} withCopy allowScroll /> |
There was a problem hiding this comment.
why do we need join? the parser should already have formatted logs, right? Also refer - https://github.com/appwrite/console/blob/main/src/routes/(console)/project-%5Bregion%5D-%5Bproject%5D/settings/migrations/details.svelte
2 participants
What does this PR do?
csvImportBox.svelteModalandCodeTest Plan
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?
yes
Summary by CodeRabbit
New Features
Style
Chores